| Summary: | constraint bug in tests/ppc64/test_isa_2_07_part1.c for mtfprwa | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | Mark Wielaard <mark> |
| Component: | general | Assignee: | Julian Seward <jseward> |
| Status: | CLOSED FIXED | ||
| Severity: | normal | CC: | bergner, cel, will_schmidt |
| Priority: | NOR | ||
| Version First Reported In: | 3.13.0 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed/Implemented In: | ||
| Sentry Crash Report: | |||
I'll note the same fix needs to be made to the 32-bit test case too: none/tests/ppc32/test_isa_2_07_part1.c I did the regression testing and some hand testing of the change to independently verify the change is OK. Note, the source for the test is in none/tests/ppc32/test_isa_2_07_part1.c. The entry none/tests/ppc64/test_isa_2_07_part1.c is just a link to the file in the ppc32 directory. So, one change does both 32-bit and 64-bit. Mark go ahead and commit the change. From IRC, Mark asked that I do the commit. I will do the commit. Change committed. commit 03c97360866c027bcc77d5c4872e66032dac3671 Author: Carl Love <carll@us.ibm.com> Date: Tue Feb 27 12:41:43 2018 -0600 Bug 391164: constraint bug in tests/ppc64/test_isa_2_07_part1.c for mtfprwa Fix destination register constraint in assembly code in function test_mtfprwa in file none/tests/ppc64/test_isa_2_07_part1.c. Constraint changed from "=ws" to "=d". Added some missing test cases to test_isa_2_01_part1.c. commit 9afb136f591c59bfb138e5b141ca59fe0d9e04af Author: Carl Love <carll@us.ibm.com> Date: Tue Feb 27 17:52:01 2018 -0600 PPC64, Missing tests for mtvsrwa, mtvrwa, mtvrd, and mtfprd. Add the missing tests to none/tests/ppc64/test_isa_2_07_part1.c. Update the expected output file with the new test outputs. Peter noted that I had a couple register constraint issues with the new tests. The errors were fixed in the following Valgrind commit. commit 839b244af27d6b7d27001c10ed308c485fecd3a8 Author: Carl Love <carll@us.ibm.com> Date: Thu Mar 1 13:50:56 2018 -0600 PPC64, Fix tests for mtvsrwa and mtfprd. Fix the register constraints for the vtvsrwa and mtfprd instructions in test_isa_2_07_part2.c. Update the expected output in none/tests/jm_vec_isa_2_07.stdout.exp. Carl, is this OK to close now? (In reply to Julian Seward from comment #7) > Carl, is this OK to close now? Yes, this should be good to be closed now. (Reported issue is resolved, no outstanding issues with the fixes). Issue is fixed, closing |
Building on Fedora rawhide with gcc 8 and binutils 2.30 produces the following error: gcc -DHAVE_CONFIG_H -I. -I../../.. -I../../.. -I../../../include -I../../../coregrind -I../../../include -I../../../VEX/pub -I../../../VEX/pub -DVGA_ppc64le=1 -DVGO_linux=1 -DVGP_ppc64le_linux=1 -DVGPV_ppc64le_linux_vanilla=1 -Winline -Wall -Wshadow -Wno-long-long -g -fno-stack-protector -m64 -Winline -Wall -O -g -mregnames -DHAS_ISA_2_07 -m64 -mcpu=power8 -MT test_isa_2_07_part1-test_isa_2_07_part1.o -MD -MP -MF .deps/test_isa_2_07_part1-test_isa_2_07_part1.Tpo -c -o test_isa_2_07_part1-test_isa_2_07_part1.o `test -f 'test_isa_2_07_part1.c' || echo './'`test_isa_2_07_part1.c /tmp/ccWcWDpE.s: Assembler messages: /tmp/ccWcWDpE.s:376: Warning: invalid register expression /tmp/ccWcWDpE.s:376: Error: operand out of range (32 is not between 0 and 31) make[5]: *** [Makefile:1140: test_isa_2_07_part1-test_isa_2_07_part1.o] Error 1 This is caused by: static vector unsigned long long vec_out static void test_mtfprwa (void) { __asm__ __volatile__ ("mtfprwa %x0,%1" : "=ws" (vec_out) : "r" (r14)); }; Quoting Peter Bergner: "This is a constraint bug. The mtfprwa instruction is actually an extended mnemonic for mtvsrwa which does allow a VSX register as the dest reg. However, mtfprwa is defined to write only an FPR reg, so it should be using either the %f or %d constraint." Changing the constraint from "ws" (VSX vector register to hold scalar double values or NO_REGS) to "d" (Floating point register (containing 64-bit value)) fixes the build issue and still makes the testcase pass. diff --git a/none/tests/ppc64/test_isa_2_07_part1.c b/none/tests/ppc64/test_isa_2_07_part1.c index 73a563ca5..201fa8855 100644 --- a/none/tests/ppc64/test_isa_2_07_part1.c +++ b/none/tests/ppc64/test_isa_2_07_part1.c @@ -406,7 +406,7 @@ static void test_mtvsrwz (void) static void test_mtfprwa (void) { - __asm__ __volatile__ ("mtfprwa %x0,%1" : "=ws" (vec_out) : "r" (r14)); + __asm__ __volatile__ ("mtfprwa %x0,%1" : "=d" (vec_out) : "r" (r14)); }; static test_t tests_move_ops_spe[] = {