| Message ID | 1d526ceb-716b-4c64-bf3b-bb9a1222d0aa@BAMAIL02.ba.imgtec.org |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Steve Ellcey [mailto:sellcey@imgtec.com] > Sent: Tuesday, January 26, 2016 3:43 PM > To: gcc-patches@gcc.gnu.org > Cc: matthew.fortune@imgtec.com; Moore, Catherine > Subject: [Patch, MIPS] Patch for PR 68400, a mips16 bug > > Here is a patch for PR6400. The problem is that and_operands_ok was > checking one operand to see if it was a memory_operand but MIPS16 > addressing is more restrictive than what the general memory_operand > allows. The fix was to call mips_classify_address if TARGET_MIPS16 is set > because it will do a more complete mips16 addressing check and reject > operands that do not meet the more restrictive requirements. > > I ran the GCC testsuite with no regressions and have included a test case as > part of this patch. > > OK to checkin? > > Steve Ellcey > sellcey@imgtec.com > > > 2016-01-26 Steve Ellcey <sellcey@imgtec.com> > > PR target/68400 > * config/mips/mips.c (and_operands_ok): Add MIPS16 check. > > > This is OK. Thanks, Catherine
"Steve Ellcey " <sellcey@imgtec.com> writes: > Here is a patch for PR6400. The problem is that and_operands_ok was checking > one operand to see if it was a memory_operand but MIPS16 addressing is more > restrictive than what the general memory_operand allows. The fix was to > call mips_classify_address if TARGET_MIPS16 is set because it will do a > more complete mips16 addressing check and reject operands that do not meet > the more restrictive requirements. > > I ran the GCC testsuite with no regressions and have included a test case as > part of this patch. > > OK to checkin? > > Steve Ellcey > sellcey@imgtec.com > > > 2016-01-26 Steve Ellcey <sellcey@imgtec.com> > > PR target/68400 > * config/mips/mips.c (and_operands_ok): Add MIPS16 check. > > > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c > index dd54d6a..adeafa3 100644 > --- a/gcc/config/mips/mips.c > +++ b/gcc/config/mips/mips.c > @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask, rtx shift, int maxlen) > bool > and_operands_ok (machine_mode mode, rtx op1, rtx op2) > { > - return (memory_operand (op1, mode) > - ? and_load_operand (op2, mode) > - : and_reg_operand (op2, mode)); > + > + if (memory_operand (op1, mode)) > + { > + if (TARGET_MIPS16) { > + struct mips_address_info addr; > + if (!mips_classify_address (&addr, op1, mode, false)) > + return false; > + } Nit: brace formatting. It looks like the patch only works by accident. The code above is passing the MEM, rather than the address inside the MEM, to mips_classify_address. Since (mem (mem ...)) is never valid on MIPS, the effect is to disable the memory alternatives of *and<mode>3_mips16 unconditionally. The addresses that occur in the testcase are valid as far as mips_classify_address is concerned. FWIW, there shouldn't be any difference between the addresses that memory_operand accepts and the addresses that mips_classify_address accepts. In theory, the "W" constraints in *and<mode>3_mips16 are supposed to tell the target-independent code that this instruction cannot handle constant addresses or stack-based addresses. That seems to be working correctly during RA for the testcase. The problem comes in regcprop, which ends up creating a second stack pointer rtx distinct from stack_pointer_rtx: (reg/f:SI 29 $sp [375]) (Note the ORIGINAL_REGNO of 375.) This then defeats the test in mips_stack_address_p: bool mips_stack_address_p (rtx x, machine_mode mode) { struct mips_address_info addr; return (mips_classify_address (&addr, x, mode, false) && addr.type == ADDRESS_REG && addr.reg == stack_pointer_rtx); } Change the == to rtx_equal_p and the test passes. I don't think that's the correct fix though -- the fix is to stop a second stack pointer rtx from being created. Thanks, Richard
Richard Sandiford <rdsandiford@googlemail.com> writes: > "Steve Ellcey " <sellcey@imgtec.com> writes: > > Here is a patch for PR6400. The problem is that and_operands_ok was checking > > one operand to see if it was a memory_operand but MIPS16 addressing is more > > restrictive than what the general memory_operand allows. The fix was to > > call mips_classify_address if TARGET_MIPS16 is set because it will do a > > more complete mips16 addressing check and reject operands that do not meet > > the more restrictive requirements. > > > > I ran the GCC testsuite with no regressions and have included a test case as > > part of this patch. > > > > OK to checkin? > > > > Steve Ellcey > > sellcey@imgtec.com > > > > > > 2016-01-26 Steve Ellcey <sellcey@imgtec.com> > > > > PR target/68400 > > * config/mips/mips.c (and_operands_ok): Add MIPS16 check. > > > > > > > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c > > index dd54d6a..adeafa3 100644 > > --- a/gcc/config/mips/mips.c > > +++ b/gcc/config/mips/mips.c > > @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask, rtx shift, int > maxlen) > > bool > > and_operands_ok (machine_mode mode, rtx op1, rtx op2) > > { > > - return (memory_operand (op1, mode) > > - ? and_load_operand (op2, mode) > > - : and_reg_operand (op2, mode)); > > + > > + if (memory_operand (op1, mode)) > > + { > > + if (TARGET_MIPS16) { > > + struct mips_address_info addr; > > + if (!mips_classify_address (&addr, op1, mode, false)) > > + return false; > > + } > > Nit: brace formatting. > > It looks like the patch only works by accident. The code above > is passing the MEM, rather than the address inside the MEM, to > mips_classify_address. Since (mem (mem ...)) is never valid on MIPS, > the effect is to disable the memory alternatives of *and<mode>3_mips16 > unconditionally. > > The addresses that occur in the testcase are valid as far as > mips_classify_address is concerned. FWIW, there shouldn't be any > difference between the addresses that memory_operand accepts and the > addresses that mips_classify_address accepts. > > In theory, the "W" constraints in *and<mode>3_mips16 are supposed to > tell the target-independent code that this instruction cannot handle > constant addresses or stack-based addresses. That seems to be working > correctly during RA for the testcase. The problem comes in regcprop, > which ends up creating a second stack pointer rtx distinct from > stack_pointer_rtx: > > (reg/f:SI 29 $sp [375]) > > (Note the ORIGINAL_REGNO of 375.) This then defeats the test in > mips_stack_address_p: > > bool > mips_stack_address_p (rtx x, machine_mode mode) > { > struct mips_address_info addr; > > return (mips_classify_address (&addr, x, mode, false) > && addr.type == ADDRESS_REG > && addr.reg == stack_pointer_rtx); > } > > Change the == to rtx_equal_p and the test passes. I don't think that's > the correct fix though -- the fix is to stop a second stack pointer rtx > from being created. Agreed, though I'm inclined to say do both. We actually hit this same issue while testing some 4.9.2 based tools recently but I hadn't got confirmation from Andrew (cc'd) whether it was definitely the same issue. Andrew fixed this by updating all tests against stack_pointer_rtx to compare register numbers instead (but rtx_equal_p is better still). I think it is worthwhile looking in more detail why a new stack pointer rtx appears. Andrew did look briefly at the time but I don't recall his conclusions... Matthew
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index dd54d6a..adeafa3 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -8006,9 +8006,18 @@ mask_low_and_shift_p (machine_mode mode, rtx mask, rtx shift, int maxlen) bool and_operands_ok (machine_mode mode, rtx op1, rtx op2) { - return (memory_operand (op1, mode) - ? and_load_operand (op2, mode) - : and_reg_operand (op2, mode)); + + if (memory_operand (op1, mode)) + { + if (TARGET_MIPS16) { + struct mips_address_info addr; + if (!mips_classify_address (&addr, op1, mode, false)) + return false; + } + return and_load_operand (op2, mode); + } + else + return and_reg_operand (op2, mode); } /* The canonical form of a mask-low-and-shift-left operation is 2016-01-26 Steve Ellcey <sellcey@imgtec.com> PR target/68400 * gcc.target/mips/mips.exp (mips_option_groups): Add stack-protector. * gcc.target/mips/pr68400.c: New test. diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp index f191331..ff9c99a 100644 --- a/gcc/testsuite/gcc.target/mips/mips.exp +++ b/gcc/testsuite/gcc.target/mips/mips.exp @@ -257,6 +257,7 @@ set mips_option_groups { lsa "(|!)HAS_LSA" section_start "-Wl,--section-start=.*" frame-header "-mframe-header-opt|-mno-frame-header-opt" + stack-protector "-fstack-protector" } for { set option 0 } { $option < 32 } { incr option } { diff --git a/gcc/testsuite/gcc.target/mips/pr68400.c b/gcc/testsuite/gcc.target/mips/pr68400.c index e69de29..1099568 100644 --- a/gcc/testsuite/gcc.target/mips/pr68400.c +++ b/gcc/testsuite/gcc.target/mips/pr68400.c @@ -0,0 +1,28 @@ +/* PR target/pr68400 + This was triggering an ICE in change_address_1 when compiled with -Os. */ + +/* { dg-do compile } */ +/* { dg-options "-fstack-protector -mips16" } */ + +typedef struct s { + unsigned long long d; + long long t; +} p; + +int sh(int x, unsigned char *buf) +{ + p *uhdr = (p *)buf; + unsigned int i = 0; + uhdr->d = ((uhdr->d & 0xff00000000000000LL) >> 56) + | ((uhdr->d & 0x0000ff0000000000LL) >> 24) + | ((uhdr->d & 0x00000000ff000000LL) << 8) + | ((uhdr->d & 0x00000000000000ffLL) << 56); + uhdr->t = ((uhdr->t & 0xff00000000000000LL) >> 56) + | ((uhdr->t & 0x0000ff0000000000LL) >> 24) + | ((uhdr->t & 0x000000ff00000000LL) >> 8) + | ((uhdr->t & 0x00000000ff000000LL) << 8) + | ((uhdr->t & 0x000000000000ff00LL) << 40) + | ((uhdr->t & 0x00000000000000ffLL) << 56); + i += 4; + if (x < i) return 0; else return 1; +}