Created attachment 50066 [details] Patch to complete IBM ISA 2.05 (i.e., POWER6) support Currently, Valgrind has only partial support for new IBM POWER6 instructions (as defined in http://www.power.org/resources/reading/PowerISA_V2.05.pdf). The attached patch completes the 2.05 support. Some things to note about this patch: - It was created against an August 11, 2010 snapshot of Valgrind SVN. - Applying the patch will result in an expected failure involving none/tests/ppc64/jm-insns.c, since this file is symbolically linked to none/tests/ppc32/jm-insns.c, which is patched earlier on during the patch application. Just hit "Enter" twice to ignore that failure, and the rest of the patch should apply cleanly. - This patch contains some new shell scripts which need to be changed to executable before attempting to run the testsuite. These new script files are: - memcheck/tests/ppc32/filter_stderr - memcheck/tests/ppc64/filter_stderr - none/tests/ppc32/check_vmx_cap - none/tests/ppc64/check_vmx_cap And, of course, these files should be made executable before being checking into SVN. The results of running 'make regtest' on a POWER6 box are almost identical before and after applying this patch -- about 50 failing testcases. I have looked into many of those failing testcases and, so far, have been able to cut the number of failures to less than half that. I will attach a separate testsuite patch that provides the improved testsuite results.
Created attachment 50067 [details] Testsuite fixes to clean up many bogus ppc64 regtest failures Like the ISA 2.05 support patch, this patch also should apply cleanly to an August 11, 2010 snapshot of Valgrind SVN, with the same exception that there will be a rejection for the none/tests/ppc64/jm-insns.c file that's symbolically linked to none/tests/ppc32/jm-insns.c. Simply hit "Enter" twice to ignore that rejection.
(In reply to comment #0) > Created an attachment (id=50066) [details] > Patch to complete IBM ISA 2.05 (i.e., POWER6) support Mostly it looks good. But this looks a bit dubious. @@ -2907,9 +2911,9 @@ static Bool dis_int_cmp ( UInt theInstr IRExpr *a = getIReg(rA_addr); IRExpr *b; - if (!mode64 && flag_L==1) { // L==1 invalid for 32 bit. - vex_printf("dis_int_cmp(ppc)(flag_L)\n"); - return False; + if (!mode64 && flag_L==1) { // L==1 probably not valid for 32 bit + vex_printf("*** WARNING ***\n"); + vex_printf("\tProbable error doing 64-bit integer compare in 32-bit mode\n"); } What's the deal here? Is it or is it not an allowable instruction in 32-bit mode? If it is, what is the behaviour supposed to be?
(In reply to comment #0) > Created an attachment (id=50066) [details] > Patch to complete IBM ISA 2.05 (i.e., POWER6) support Committed (vex: r2029, minus the part shown in comment #2, and tests as valgrind r11338, with check_vmx_cap moved to tests/).
(In reply to comment #1) > Created an attachment (id=50067) [details] > Testsuite fixes to clean up many bogus ppc64 regtest failures This is definitely a good thing to do. I've got some comments on the patch, though: * non_arch_ppc is not the correct test for whether exp-ptrcheck will work on a given platform, since ARM is now supported, exp-ptrcheck doesn't work for that either, yet it is non-ppc. Really, need "is_ptrcheck_supported_arch". * (renamed) non_arch_ppc needs to be in the toplevel tests/ along with the other test framework scripts * I didn't understand the purpose of the changes in none/tests/ppc{32,64}/jm-insns.c. Why are you redefining uint32_t and uint64_t? What does this have to do with the vector keyword mentioned in the (augmented) comment?
(In reply to comment #2) > (In reply to comment #0) > > Created an attachment (id=50066) [details] [details] > > Patch to complete IBM ISA 2.05 (i.e., POWER6) support > > Mostly it looks good. But this looks a bit dubious. > > @@ -2907,9 +2911,9 @@ static Bool dis_int_cmp ( UInt theInstr > IRExpr *a = getIReg(rA_addr); > IRExpr *b; > > - if (!mode64 && flag_L==1) { // L==1 invalid for 32 bit. > - vex_printf("dis_int_cmp(ppc)(flag_L)\n"); > - return False; > + if (!mode64 && flag_L==1) { // L==1 probably not valid for 32 bit > + vex_printf("*** WARNING ***\n"); > + vex_printf("\tProbable error doing 64-bit integer compare in 32-bit > mode\n"); > } > > What's the deal here? Is it or is it not an allowable instruction in > 32-bit mode? If it is, what is the behaviour supposed to be? It is not. Thanks for catching this error. I inadvertently removed the "return False" statement. The reason for the warning message was to make it more clear to the user that their assembly code was doing something bogus.
(In reply to comment #4) > (In reply to comment #1) > > Created an attachment (id=50067) [details] [details] > > Testsuite fixes to clean up many bogus ppc64 regtest failures > > This is definitely a good thing to do. I've got some comments on > the patch, though: > > * non_arch_ppc is not the correct test for whether exp-ptrcheck will > work on a given platform, since ARM is now supported, exp-ptrcheck > doesn't work for that either, yet it is non-ppc. Really, need > "is_ptrcheck_supported_arch". > > * (renamed) non_arch_ppc needs to be in the toplevel tests/ along with > the other test framework scripts OK. Would you like me to re-submit the patch with these changes? > > * I didn't understand the purpose of the changes in > none/tests/ppc{32,64}/jm-insns.c. Why are you redefining uint32_t > and uint64_t? What does this have to do with the vector keyword > mentioned in the (augmented) comment? Without this change, I get the following compile error with gcc 4.3 (e.g., on SLES 11 SP1): --------------------- jm-insns.c: In function 'register_vfarg': jm-insns.c:4438: error: 'vector' undeclared (first use in this function) jm-insns.c:4438: error: (Each undeclared identifier is reported only once jm-insns.c:4438: error: for each function it appears in.) jm-insns.c:4438: error: expected ';' before 'uint32_t' jm-insns.c:4441: error: 'vfargI' undeclared (first use in this function) jm-insns.c:4441: error: expected ')' before 'uint32_t' jm-insns.c:4441: error: expected ';' before '{' token make[1]: *** [jm_insns-jm-insns.o] Error 1 -------------------------- Our resident gcc expert explained it to me thusly: "This is a symptom of the way 4.3 implements vector to be like a conditional keyword, so that in theory if you use it normally in non-vector code you won't even know it exists (shades of PL/1), but if you put it in a declaration where the next thing is float, int, etc. it will instead convert the type to the appropriate vector type. In GCC 4.3 __vector is implemented as a macro that emits the appropriate attribute declaration, and then vector is a conditional that becomes either __vector or just itself, based on what the next token is. Needless to say this is a little fragile, and needs to be retweaked every so often. In GCC 4.5 this is implemented as a conditional keyword, and is somewhat more robust." He goes on to say that the Altivec manual section 2.2 (in GCC doc) states that the syntax does not allow the use of a typedef name as a type specifier. So that's why my patch undef's the unit[32|64]_t typedefs and makes #defines for them instead.
(In reply to comment #6) > OK. Would you like me to re-submit the patch with these changes? Yes, please do. > > * I didn't understand the purpose of the changes in > > none/tests/ppc{32,64}/jm-insns.c. Why are you redefining uint32_t > > and uint64_t? What does this have to do with the vector keyword > > mentioned in the (augmented) comment? > Without this change, I get the following compile error with gcc 4.3 (e.g., on > SLES 11 SP1): > --------------------- > jm-insns.c: In function 'register_vfarg': > jm-insns.c:4438: error: 'vector' undeclared (first use in this function) > jm-insns.c:4438: error: (Each undeclared identifier is reported only once > jm-insns.c:4438: error: for each function it appears in.) > jm-insns.c:4438: error: expected ';' before 'uint32_t' > jm-insns.c:4441: error: 'vfargI' undeclared (first use in this function) > jm-insns.c:4441: error: expected ')' before 'uint32_t' > jm-insns.c:4441: error: expected ';' before '{' token > make[1]: *** [jm_insns-jm-insns.o] Error 1 > -------------------------- Now you mention it, I see that failure on Rich Coe's nightly build log for ppc32 ("[Valgrind-developers] 2010-09-07 23:26:01 CDT nightly build (ppc32, Linux 2.6.27.45-0.1-default ppc)") > Our resident gcc expert explained it to me thusly: Thanks for the explanation. I never knew about this before. It would be good if you could copy it into the patch as a comment.
Created attachment 51440 [details] Version 2 of testsuite patch This version of the testsuite patch addresses the review comments Julian had.
(In reply to comment #8) > Created an attachment (id=51440) [details] > Version 2 of testsuite patch > > This version of the testsuite patch addresses the review comments Julian had. Applied jm-insns.c modifications as r11358.
exp-ptrcheck regression tests have been disabled on PowerPC and ARM in r11360 + r11362. Applied exp-bbv changes as r11361. Note: even with the attached patch applied, I still see the test_gx test fail: --- test_gx.stdout.exp 2010-09-18 04:23:29.000000000 -0400 +++ test_gx.stdout.out 2010-09-18 05:33:18.000000000 -0400 @@ -24,20 +24,20 @@ fres -inf -> -0.0e+00 fres nan -> nan fres nan -> nan -fres -5.000000e+100 -> -0.0e+00 +fres -5.000000e+100 -> -2.0e-101 fres -5.000000e+20 -> -2.0e-21 fres -5.010000e+02 -> -2.0e-03 fres -6.000000e+00 -> -1.7e-01 fres -1.010000e+00 -> -9.9e-01 fres -2.000000e-20 -> -5.0e+19 -fres -2.000000e-200 -> -inf -fres 2.000000e-200 -> inf +fres -2.000000e-200 -> -5.0e+199 +fres 2.000000e-200 -> 5.0e+199 fres 2.000000e-20 -> 5.0e+19 fres 1.010000e+00 -> 9.9e-01 fres 6.000000e+00 -> 1.7e-01 fres 5.010000e+02 -> 2.0e-03 fres 5.000000e+20 -> 2.0e-21 -fres 5.000000e+100 -> 0.0e+00 +fres 5.000000e+100 -> 2.0e-101 frsqrte 0.000000e+00 -> inf frsqrte inf -> 0
Applied memcheck/tests/origin5-bz2.stderr.exp-glibc27-ppc64 patch as r11363.
Added memcheck/tests/varinfo?.stderr.exp-ppc64 in r11364.
I'd like to close this, but I'm a bit concerned about the regressions Bart mentioned in comment 10. Perhaps test_gx is testing implementation- dependent behaviour in fres.
(In reply to comment #13) > I'd like to close this, but I'm a bit concerned about the regressions > Bart mentioned in comment 10. Perhaps test_gx is testing implementation- > dependent behaviour in fres. I think I'll have time to check into this today.
(In reply to comment #10) > exp-ptrcheck regression tests have been disabled on PowerPC and ARM in r11360 + > r11362. > Applied exp-bbv changes as r11361. > > Note: even with the attached patch applied, I still see the test_gx test fail: > > --- test_gx.stdout.exp 2010-09-18 04:23:29.000000000 -0400 > +++ test_gx.stdout.out 2010-09-18 05:33:18.000000000 -0400 > @@ -24,20 +24,20 @@ > fres -inf -> -0.0e+00 > fres nan -> nan > fres nan -> nan > -fres -5.000000e+100 -> -0.0e+00 > +fres -5.000000e+100 -> -2.0e-101 > fres -5.000000e+20 -> -2.0e-21 > fres -5.010000e+02 -> -2.0e-03 > fres -6.000000e+00 -> -1.7e-01 > fres -1.010000e+00 -> -9.9e-01 > fres -2.000000e-20 -> -5.0e+19 > -fres -2.000000e-200 -> -inf > -fres 2.000000e-200 -> inf > +fres -2.000000e-200 -> -5.0e+199 > +fres 2.000000e-200 -> 5.0e+199 > fres 2.000000e-20 -> 5.0e+19 > fres 1.010000e+00 -> 9.9e-01 > fres 6.000000e+00 -> 1.7e-01 > fres 5.010000e+02 -> 2.0e-03 > fres 5.000000e+20 -> 2.0e-21 > -fres 5.000000e+100 -> 0.0e+00 > +fres 5.000000e+100 -> 2.0e-101 > > frsqrte 0.000000e+00 -> inf > frsqrte inf -> 0 Bart, I've not found a processor/distro combination that reproduces this problem. What processor model and distro are you seeing this on?
(In reply to comment #15) > Bart, I've not found a processor/distro combination that reproduces this > problem. What processor model and distro are you seeing this on? Please let me know if you need more information than what is mentioned below: $ uname -a Linux cell11 2.6.25.14-108.20080910bsc.ppc64 #1 SMP Fri Sep 12 11:44:36 CEST 2008 ppc64 ppc64 ppc64 GNU/Linux $ cat /etc/issue.net Fedora release 9 (Sulphur) Kernel \r on an \m (\l) $ cat /proc/cpuinfo processor : 0 cpu : Cell Broadband Engine, altivec supported clock : 3200.000000MHz revision : 5.1 (pvr 0070 0501)
Update: the aforementioned Cell CPU does not support at least one of the fre* instructions natively, as is clear from the following output: $ none/tests/ppc32/test_gx Illegal instruction
Closing. I'm pretty convinced this stuff works OK. Please reopen if not.