| Message ID | 20160615194620.GN7387@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/15/2016 01:46 PM, Jakub Jelinek wrote: > Hi! > > As the following testcase shows, CSE mishandles const/pure calls, it assumes > that const/pure calls can't clobber even their argument slots. But, the > argument slots are owned by the callee, so need to be volatile across the > calls. On the testcase the second round of argument stores is eliminated, > because CSE thinks those memory slots already have the right values. > > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for > trunk? > > 2016-06-15 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/71532 > * cse.c (cse_insn): For const/pure calls, invalidate argument passing > memory slots. > > * gcc.dg/torture/pr71532.c: New test. FWIW I don't think ownership of the argument slots has ever been definitively addressed by any ABI and it's been an open question in my mind for 20+ years -- though I've largely leaned towards callee ownership on my own thinking. In an ideal world we'd push to get this clarified at the ABI level. Certainly this is the safe thing to do. OK for the trunk. Jeff
On Wed, Jun 15, 2016 at 04:03:04PM -0600, Jeff Law wrote: > FWIW I don't think ownership of the argument slots has ever been > definitively addressed by any ABI and it's been an open question in my mind > for 20+ years -- though I've largely leaned towards callee ownership on my > own thinking. In an ideal world we'd push to get this clarified at the ABI > level. The PowerPC64 ABI specifies that the stack parameter save area is not preserved over calls. There's a good reason for this: An ABI that specifies stack argument slots as preserved over calls cannot allow sibling calls.
On 06/15/2016 07:14 PM, Alan Modra wrote: > On Wed, Jun 15, 2016 at 04:03:04PM -0600, Jeff Law wrote: >> FWIW I don't think ownership of the argument slots has ever been >> definitively addressed by any ABI and it's been an open question in my mind >> for 20+ years -- though I've largely leaned towards callee ownership on my >> own thinking. In an ideal world we'd push to get this clarified at the ABI >> level. > > The PowerPC64 ABI specifies that the stack parameter save area is not > preserved over calls. There's a good reason for this: An ABI that > specifies stack argument slots as preserved over calls cannot allow > sibling calls. I didn't know it was specified there. Excellent. Good to see someone doing the right thing with getting that nailed down. I did read your message about sibling call support implying callee ownership of the slots -- and I agree with that conclusion. jeff
--- gcc/cse.c.jj 2016-05-20 09:05:08.000000000 +0200 +++ gcc/cse.c 2016-06-15 16:01:21.568523552 +0200 @@ -5751,6 +5751,13 @@ cse_insn (rtx_insn *insn) { if (!(RTL_CONST_OR_PURE_CALL_P (insn))) invalidate_memory (); + else + /* For const/pure calls, invalidate any argument slots, because + those are owned by the callee. */ + for (tem = CALL_INSN_FUNCTION_USAGE (insn); tem; tem = XEXP (tem, 1)) + if (GET_CODE (XEXP (tem, 0)) == USE + && MEM_P (XEXP (XEXP (tem, 0), 0))) + invalidate (XEXP (XEXP (tem, 0), 0), VOIDmode); invalidate_for_call (); } --- gcc/testsuite/gcc.dg/torture/pr71532.c.jj 2016-06-15 16:37:55.340368699 +0200 +++ gcc/testsuite/gcc.dg/torture/pr71532.c 2016-06-15 16:39:45.841956435 +0200 @@ -0,0 +1,39 @@ +/* PR rtl-optimization/71532 */ +/* { dg-do run } */ +/* { dg-additional-options "-mtune=slm" { target i?86-*-* x86_64-*-* } } */ + +__attribute__((noinline, noclone, pure)) int +foo (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k, int l) +{ + a++; b++; c++; d++; e++; f++; g++; h++; i++; j++; k++; l++; + asm volatile ("" : : "g" (&a), "g" (&b), "g" (&c), "g" (&d) : "memory"); + asm volatile ("" : : "g" (&e), "g" (&f), "g" (&g), "g" (&h) : "memory"); + asm volatile ("" : : "g" (&i), "g" (&j), "g" (&k), "g" (&l) : "memory"); + return a + b + c + d + e + f + g + h + i + j + k + l; +} + +__attribute__((noinline, noclone, pure)) int +bar (int a, int b, int c, int d, int e, int f, int g, int h, int i, int j, int k, int l) +{ + a++; b++; c++; d++; e++; f++; g++; h++; i++; j++; k++; l++; + asm volatile ("" : : "g" (&a), "g" (&b), "g" (&c), "g" (&d) : "memory"); + asm volatile ("" : : "g" (&e), "g" (&f), "g" (&g), "g" (&h) : "memory"); + asm volatile ("" : : "g" (&i), "g" (&j), "g" (&k), "g" (&l) : "memory"); + return 2 * a + b + c + d + e + f + g + h + i + j + k + l; +} + +__attribute__((noinline, noclone)) int +test () +{ + int a = foo (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12); + a += bar (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12); + return a; +} + +int +main () +{ + if (test () != 182) + __builtin_abort (); + return 0; +}