Bug 391164 - constraint bug in tests/ppc64/test_isa_2_07_part1.c for mtfprwa
Summary: constraint bug in tests/ppc64/test_isa_2_07_part1.c for mtfprwa
Status: CLOSED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: 3.13.0
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-27 13:28 UTC by Mark Wielaard
Modified: 2018-08-14 16:21 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Wielaard 2018-02-27 13:28:52 UTC
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[] = {
Comment 1 Peter Bergner 2018-02-27 15:45:28 UTC
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
Comment 2 Carl Love 2018-02-27 17:59:45 UTC
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.
Comment 3 Carl Love 2018-02-27 18:02:36 UTC
From IRC, Mark asked that I do the commit.  I will do the commit.
Comment 4 Carl Love 2018-02-27 18:43:02 UTC
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".
Comment 5 Carl Love 2018-02-28 21:28:35 UTC
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.
Comment 6 Carl Love 2018-03-01 20:01:57 UTC
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.
Comment 7 Julian Seward 2018-08-06 08:56:04 UTC
Carl, is this OK to close now?
Comment 8 Will Schmidt 2018-08-06 15:30:04 UTC
(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).
Comment 9 Carl Love 2018-08-14 16:21:13 UTC
Issue is fixed, closing