User account creation filtered due to spam.

Bug 56470 - [4.7 Regression] ICE output_operand: invalid shift operand
Summary: [4.7 Regression] ICE output_operand: invalid shift operand
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.8.0
: P2 normal
Target Milestone: 4.7.3
Assignee: Richard Earnshaw
URL:
Keywords: ice-on-valid-code
Depends on:
Blocks:
 
Reported: 2013-02-27 09:39 UTC by Jakub Jelinek
Modified: 2013-03-18 11:54 UTC (History)
0 users

See Also:
Host:
Target: arm
Build:
Known to work:
Known to fail:
Last reconfirmed: 2013-03-04 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jakub Jelinek 2013-02-27 09:39:46 UTC
int w[5];

void
foo (int x, unsigned y)
{
  long long r = 0;
  long long s;
  int i;
  switch (x)
    {
    case 1:
      for (i = 0; i < 4; i++)
        {
          if (!y)
            s = (w[9] << (10 - 2 * i)) >> (32 - 5 * i + 5 * i);
          r |= (s & 0xffffffff) << (i * 5);
        }
      break;
    case 3:
      r = w[x] >> y;
    }
  w[x] = r;
}

ICEs at -O2 on arm-linux-gnuabi.  This reduced testcase ICEs also in 4.7, but
the unreduced one at https://bugzilla.redhat.com/show_bug.cgi?id=915830#c1
only ICEs in 4.8 and not in 4.7.
I wonder why arm backend uses constraints (N) to enfore the shift count in the range, rather than e.g. masking the operand & 31 if CONST_INT before printing.
Shift counts >= 32 for SImode are of course undefined behavior, so it really doesn't matter what gets emitted, but we shouldn't ICE on it.
Comment 1 Richard Earnshaw 2013-03-04 07:01:07 UTC
Confirmed. 

I'm a bit wary of just truncating the value.  Shifts by 32 may be valid in the ARM back-end in some circumstances where we're using the shift as part of setting up the flags.

Fixing this fully would require getting rid of "shift_operator" and replacing it with iterators.  But that's a pretty radical overhaul.  Long term that might well be worthwhile, but not this close to a release.
Comment 2 Jakub Jelinek 2013-03-04 08:17:56 UTC
In any case, this looks like a clear backend bug, as it allows const_int 32 in the operand (through the use of M constraint), but then doesn't handle it at all in arm_output_shift (it forces use of %S3 on it and that requires 0 .. 31 constant, not 0 .. 32).  And while the code has undefined behavior, if you never invoke it, you should still be able to have it in a valid program.
Comment 3 Richard Earnshaw 2013-03-05 07:01:42 UTC
Definitely a back-end bug.  I'm not disputing that.

My surprise is that this hasn't bitten us long before now, since the code has been this way for well over ten years.
Comment 4 Jakub Jelinek 2013-03-05 09:14:46 UTC
If the folder or GIMPLE passes see it is a shift by 32, they fold it away (with or without a warning), so the fact that it is a constant larger than bitsize of the shifted operand has to be seen only during RTL optimizations.
Comment 5 Richard Earnshaw 2013-03-06 10:27:03 UTC
Working on a fix.
Comment 6 Richard Biener 2013-03-08 15:46:00 UTC
According to reporter also broken on the 4.7 branch.
Comment 7 Richard Earnshaw 2013-03-11 11:48:45 UTC
Author: rearnsha
Date: Mon Mar 11 11:48:34 2013
New Revision: 196595

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196595
Log:
	PR target/56470
	* arm.c (shift_op): Validate RTL pattern on the fly.
	(arm_print_operand, case 'S'): Don't use shift_operator to validate
	the RTL.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c
Comment 8 Richard Earnshaw 2013-03-11 14:38:14 UTC
Fixed on trunk.  Testing back-port to 4.7.
Comment 9 Richard Earnshaw 2013-03-18 11:52:15 UTC
Author: rearnsha
Date: Mon Mar 18 11:52:08 2013
New Revision: 196780

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=196780
Log:
	PR target/56470
	* arm.c (shift_op): Validate RTL pattern on the fly.
	(arm_print_operand, case 'S'): Don't use shift_operator to validate
	the RTL.

Modified:
    branches/gcc-4_7-branch/gcc/ChangeLog
    branches/gcc-4_7-branch/gcc/config/arm/arm.c
Comment 10 Richard Earnshaw 2013-03-18 11:54:13 UTC
Fixed