commit 26a3abd27db2ef63dafea1ecab00e8239f341f0f Author: Eyal Soha <eyalsoha@gmail.com> Date: Thu Feb 10 09:52:54 2022 -0700 Bug 432801 - Valgrind 3.16.1 reports a jump based on uninitialized memory somehow related to clang and signals Add support for precise computation of SIMD greater-than on amd64 and x86. This adds support for 64bit, 16bit, and 8bit to the existing 32bit support. Add a more precise method to calculate the vbit values. The test results for the Iop_CmpGT64Ux2 were not updates. The Iop is only supported on PPC32 and PPC64.
Patch to update the expected results committed. commit 5396fc8d0197e843657736ef8f7d9774400980c9 (HEAD -> master, origin/master, origin/HEAD) Author: Carl Love <cel@linux.ibm.com> Date: Mon Oct 23 19:15:00 2023 -0400 Bug 476025 - Powerpc, update expected results for the vbit-test In commit: commit 26a3abd27db2ef63dafea1ecab00e8239f341f0f Author: Eyal Soha <eyalsoha@gmail.com> Date: Thu Feb 10 09:52:54 2022 -0700 Bug 432801 - Valgrind 3.16.1 reports a jump based on uninitialized memory somehow related to clang and signals Add support for precise computation of SIMD greater-than on amd64 and x86. This adds support for 64bit, 16bit, and 8bit to the existing 32bit support. The Iop_CmpGT64Ux2 is only supported on PPC32 and PPC64. The above commit adds a more precise method, expensiveCmpGT, for setting the vibits for the Iop_CmpGT64Ux2 Iop. The expected results for the vbit test were not updated to the new more precise results. This patch updates the expected results for the Iop. Patch tested on Power 10.
Issue resolved.
Closing
The patch file was all wrong. The commit was reverted. Need to rework the fix.
I'll write some details to explain what is going on here. In this commit: 26a3abd27db2ef63dafea1ecab00e8239f341f0f The handling of pcmpgt was improved. The pcmpgt operation takes two 128-bit registers and treats every (8/16/32/64) bits as a separate word and then is tests them for "greater than". Before the commit 26a3abd2, valgrind just looked for any undefined bits in one or the other register and said that if some all undefined then the whole result is undefined. valgrind was doing the wrong thing. This caused false positives for me in a newish version of clang so I put in the expensive check which now calculates the "greater than" despite undefinedness and does the best possible computation. Unrelated to that fix, the vbits test checks that vbits work correctly. It tests operations and checks that the vbits generated are correct. For many operations, it's too hard to generate the correct vbits off an input so the operation is marked as "UNDEF_UNKNOWN", indicating that the vbits for that operation are too hard to calculate so we just won't test this one. All the Iop_CmpGT... operations ( there are 2-0 of them) in the vbits test are marked as "UNDEF_UNKNOWN" except for Iop_CmpGT64Ux2. This was probably a mistake when it was written. For that one, it checks that the vbits that are output are doing the wrong thing in the way that valgrind also does the wrong thing. Basically, it was confirming that valgrind is doing the wrong thing in the way that we expect valgrind to be wrong. Well, now valgrind works and so the test is getting hung up because it's checking that valgrind is still "broken". But it isn't. I never noticed this in my testing because it only tested on ppc platforms. The solution is to first set that test to UNDEF_UNKNOWN. That'll get the tests working again. The follow-up, if we want, is to convert all those UNDEF_UNKNOWNS into UNDEF_somethingelse that actually checks correctness. Now, is that the right thing to do? Maybe. On the one hand, it makes the testing more complete. On the other hand, the guy that wrote the expensiveCmpGT (me) is going to write the UNDEF_CMP_GT (me again) it's like me grading my own homework and declaring 100% on myself. Not a very robust test! However, the vbits code is in regular c++ and not the VEX IR thing so there is some value to it that way. And I can see in the vbits test that ADD, SUB, etc, *are* handled (not UNDEF_UNKNOWN) and cmpgt is similar to those. So maybe we should do it. I think that it wouldn't be too difficult to do given that we can pretty much copy the ADD and SUB examples. We'll need permuations for the various bit widths, signed/unsigned, and number of lanes. But not too bad. I'd probably still skip the floating point ones because that seems harder to me. Maybe it's not, though? I dunno.
Created attachment 162550 [details] Fix vbits for cmpgtExB where E*B==128 This adds more precise testing to the vbits program. Only amd64 is enabled. Add .ppc32=1 or .ppc64=1 to some of those and run to see if they work.
The test for the Iop_CmpGT64Ux2 was changed as follows: diff --git a/memcheck/tests/vbit-test/irops.c b/memcheck/tests/vbit-test/irops.c index a09470905..e94ea2432 100644 --- a/memcheck/tests/vbit-test/irops.c +++ b/memcheck/tests/vbit-test/irops.c @@ -861,7 +861,7 @@ static irop_t irops[] = { { DEFOP(Iop_CmpGT8Ux16, UNDEF_UNKNOWN), }, { DEFOP(Iop_CmpGT16Ux8, UNDEF_UNKNOWN), }, { DEFOP(Iop_CmpGT32Ux4, UNDEF_UNKNOWN), }, - { DEFOP(Iop_CmpGT64Ux2, UNDEF_ALL_64x2), .ppc64 = 1, .ppc32 = 1 }, + { DEFOP(Iop_CmpGT64Ux2, UNDEF_UNKNOWN), }, { DEFOP(Iop_Cnt8x16, UNDEF_UNKNOWN), }, { DEFOP(Iop_Clz8x16, UNDEF_UNKNOWN), }, { DEFOP(Iop_Clz16x8, UNDEF_UNKNOWN), }, -- The commit was: commit bb162ac6c082f371da90fb3691f3185a53c2d56f (HEAD -> master, origin/master, origin/HEAD) Author: Carl Love <cel@linux.ibm.com> Date: Wed Oct 25 11:20:30 2023 -0400 Change the vbit test specification for Iop_CmpGT64Ux2 The test should be specified as UNDEF_UNKNOWN not UNDEF_ALL_64x2. No architectures should be enaabled for the test. This effectively disables the vbit test on all architectures. Note the Iop_CmpGT* for the other sizes are already set to UNDEF_UNKNOWN. None of the Iops are getting tested at the current time. TO DO, work on adding appropriate testing for this class of Iops. Eyal has started on the new tests.
Created attachment 162565 [details] attachment-360788-0.html I completed the new tests. Please try adding ppc platforms and test them. I already added amd64 testing for a few of them. On Wed, Oct 25, 2023, 09:37 Carl Love <bugzilla_noreply@kde.org> wrote: > https://bugs.kde.org/show_bug.cgi?id=476025 > > --- Comment #7 from Carl Love <cel@us.ibm.com> --- > The test for the Iop_CmpGT64Ux2 was changed as follows: > > diff --git a/memcheck/tests/vbit-test/irops.c > b/memcheck/tests/vbit-test/irops.c > index a09470905..e94ea2432 100644 > --- a/memcheck/tests/vbit-test/irops.c > +++ b/memcheck/tests/vbit-test/irops.c > @@ -861,7 +861,7 @@ static irop_t irops[] = { > { DEFOP(Iop_CmpGT8Ux16, UNDEF_UNKNOWN), }, > { DEFOP(Iop_CmpGT16Ux8, UNDEF_UNKNOWN), }, > { DEFOP(Iop_CmpGT32Ux4, UNDEF_UNKNOWN), }, > - { DEFOP(Iop_CmpGT64Ux2, UNDEF_ALL_64x2), .ppc64 = 1, .ppc32 = 1 }, > + { DEFOP(Iop_CmpGT64Ux2, UNDEF_UNKNOWN), }, > { DEFOP(Iop_Cnt8x16, UNDEF_UNKNOWN), }, > { DEFOP(Iop_Clz8x16, UNDEF_UNKNOWN), }, > { DEFOP(Iop_Clz16x8, UNDEF_UNKNOWN), }, > -- > The commit was: > > commit bb162ac6c082f371da90fb3691f3185a53c2d56f (HEAD -> master, > origin/master, > origin/HEAD) > Author: Carl Love <cel@linux.ibm.com> > Date: Wed Oct 25 11:20:30 2023 -0400 > > Change the vbit test specification for Iop_CmpGT64Ux2 > > The test should be specified as UNDEF_UNKNOWN not UNDEF_ALL_64x2. > No architectures should be enaabled for the test. > > This effectively disables the vbit test on all architectures. Note the > Iop_CmpGT* for the other sizes are already set to UNDEF_UNKNOWN. None of > the > Iops are getting tested at the current time. > > TO DO, work on adding appropriate testing for this class of Iops. Eyal has > started on the new tests. > > -- > You are receiving this mail because: > You are on the CC list for the bug.
Created attachment 162566 [details] Fix vbits for cmpgtExB where E*B==128 rebased onto bb162ac6c Same fix as before, rebased on to the latest of master branch.
Created attachment 162779 [details] enable PowerPC testing of Iop_CmpGT* Iops Created patch to add PowerPC testing of the Iop_CmpGT* Iops. Test on top of attachment "Fix vbits for cmpgtExB where E*B==128 rebased onto bb162ac6c". The two patch series was applied on top of commit: commit bd4db67b1d386c352040b1d8fab82f5f3340fc59 (tag: VALGRIND_3_22_0, origin/master, origin/HEAD) Author: Mark Wielaard <mark@klomp.org> Date: Tue Oct 31 18:19:05 2023 +0100 -> 3.22.0 final The two patches were tested on Power 10LE, Power9BE/LE and Power 8LE. The memcheck/tests/vbit-test passes on each platform.
I think that these have all been merged.
But this one is not merged: https://bugsfiles.kde.org/attachment.cgi?id=162566 Should it be merged?
Merged commit 94e826519a810e52bba539a7c41f4074f0631e10 (HEAD -> master, origin/master, origin/HEAD) Author: Eyal Soha <eyalsoha@gmail.com> Date: Tue Oct 24 21:52:56 2023 -0600 Add support for expensive cmpgt into vbits