fix RVV index_max/min kernels returning wrong index

[Ka-zam]

index_??? should return first index meeting criteria.

[marcusmueller]

Hi @Ka-zam!

Oh! Wasn't aware that's a bug; thanks for submitting a fix! Um, are we perhaps missing a unit test there, or is the existing test borked?

[Ka-zam]

Hi @Ka-zam!

Oh! Wasn't aware that's a bug; thanks for submitting a fix! Um, are we perhaps missing a unit test there, or is the existing test borked?

Hi!
Existing test is not effective since it relies on floats being equal and in the same lane which is unlikely to happen. Adding test:

  // Test data with duplicate max values at indices 1 and 4
  float test_data[] = {0x1.0p0f, 0x1.8p1f, 0x1.0p1f, 0x1.4p1f, 0x1.8p1f, 0x1.2p1f};
  // Expected result: index 1 (first occurrence of max)

(edit)
Hmm, and removing them since they exposed similar bugs in the x64 implementations. We should decide how to handle this.

[jdemel]

Thanks for having a look at this!

So, are we consistently failing here across implementations?

size_t maxIndex = 0;
float maximum = -INFTY;
for (size_t index = 0; index < vec.size(); ++index){
  if (vec[index] > maximum){
    maximum = vec[index];
    maxIndex = index;
  }
}

I assume this would be the simple and correct algorithm. Thus the result for 1.0, 1.0, 1.0 is index 0 and not 1 or 2.

This is the kind of bug and test behavior why I introduced googletest. I'd like to add tests like this there to encode expected behavior and make it possible to test against that.

If you want to add tests for this case, please go ahead. Besides, we can merge this PR.

[Ka-zam]

Thanks for having a look at this!

So, are we consistently failing here across implementations?

size_t maxIndex = 0;
float maximum = -INFTY;
for (size_t index = 0; index < vec.size(); ++index){
  if (vec[index] > maximum){
    maximum = vec[index];
    maxIndex = index;
  }
}

I assume this would be the simple and correct algorithm. Thus the result for 1.0, 1.0, 1.0 is index 0 and not 1 or 2.

This is the kind of bug and test behavior why I introduced googletest. I'd like to add tests like this there to encode expected behavior and make it possible to test against that.

If you want to add tests for this case, please go ahead. Besides, we can merge this PR.

Yes, your implementation is how I understand it should work.

We are consistently failing across all implementations when injecting floats that are bit equivalent since none of the implementations keep track of which lane holds the minimum value. The reason RVV failed for this run was simply that the test generated identical input values spaced close enough. So, we fix and inject known problematic test patterns to test this?

[Ka-zam]

Added edge case to index_* tests:

    // Index kernels need identical values to test tie-breaking (first index wins)
    volk_test_params_t test_params_index(test_params.make_tol(0));
    test_params_index.add_float_edge_cases({
        1.0f,
        1.0f,
        1.0f,
        1.0f, // 4 identical (SSE lane width)
        1.0f,
        1.0f,
        1.0f,
        1.0f, // 8 total (AVX lane width)
        1.0f,
        1.0f,
        1.0f,
        1.0f, // 12
        1.0f,
        1.0f,
        1.0f,
        1.0f, // 16 total (AVX512 lane width)
    });
    QA(VOLK_INIT_TEST(volk_32f_index_max_16u, test_params_index))
    QA(VOLK_INIT_TEST(volk_32f_index_max_32u, test_params_index))
    QA(VOLK_INIT_TEST(volk_32f_index_min_16u, test_params_index))
    QA(VOLK_INIT_TEST(volk_32f_index_min_32u, test_params_index))

Fixed kernels:

RUN_VOLK_TESTS: volk_32fc_index_max_32u(131071,1987)
generic                          129.8275 ms ( 24073.3 MB/s)
a_avx2_variant_0                  38.0663 ms ( 82103.6 MB/s)
a_avx2_variant_1                  18.2670 ms (171094.2 MB/s)
a_sse3                            53.8067 ms ( 58085.2 MB/s)
a_avx512f                         13.9901 ms (223399.2 MB/s)
u_avx2_variant_0                  38.5189 ms ( 81138.8 MB/s)
u_avx2_variant_1                  23.3854 ms (133646.4 MB/s)
u_avx512f                         13.6445 ms (229057.8 MB/s) *
Best aligned arch:                 u_avx512f         (9.52x)
Best unaligned arch:               u_avx512f         (9.52x)
--------------------------------------------------------------------------------
.
.
.

RUN_VOLK_TESTS: volk_32f_index_min_32u(131071,997)
generic                          109.0696 ms (  9585.3 MB/s)
neon                              47.8347 ms ( 21855.7 MB/s)
neonv8                            36.2350 ms ( 28852.3 MB/s) *
Best aligned arch:                    neonv8         (3.01x)
Best unaligned arch:                  neonv8         (3.01x)
--------------------------------------------------------------------------------

[jdemel]

Thanks for noticing and fixing this bug. LGTM.

[jdemel]

I linked #700 against this PR since it discusses the same root cause. Thanks again!

[jdemel]

Merging the other PR #801 created merge conflicts. That's very unfortunate. Can you rebase?

[jdemel]

FYI: I'd like to get all your PRs #802, #803 , and #804 merged. Afterwards, it's time to do a release.

[Ka-zam]

This can be merged cleanly now.