diff mbox series

[committed] Fix rl78 newlib build failure due to bogus operand_subword_force argument

Message ID f1bbbd7c-df9e-8e30-9037-98703fd4a1a1@redhat.com
State New
Headers show
Series [committed] Fix rl78 newlib build failure due to bogus operand_subword_force argument | expand

Commit Message

Jeff Law Nov. 14, 2018, 4:35 p.m. UTC
For the attached testcase compiled with -O2 -mg10 on rl78 port we get
into expand_builtin_signbit.

EXP is exactly what you'd expect.  A CALL_EXPR to __builtin_signbit with
an argument like:

   arg:0 <ssa_name 0x7fffe9ef4678
        type <real_type 0x7fffe9e21498 double SF
            size <integer_cst 0x7fffe9e0d6c0 constant 32>
            unit-size <integer_cst 0x7fffe9e0d6d8 constant 4>
            align:16 warn_if_not_align:0 symtab:0 alias-set 1
canonical-type 0x7fffe9e21498 precision:32
            pointer_to_this <pointer_type 0x7fffe9e21888>>
        visited var <parm_decl 0x7fffe9f0c000 fp>
        def_stmt GIMPLE_NOP
        version:2>


TARGET is basically what you'd expect as well:

(gdb) p debug_rtx (target)
(reg:HI 43 [ _1 ])


There is no suitable optab and we get into this code:

 if (GET_MODE_SIZE (fmode) <= UNITS_PER_WORD)
    {
      imode = int_mode_for_mode (fmode).require ();
      temp = gen_lowpart (imode, temp);
    }
  else
    {
      imode = word_mode;
      /* Handle targets with different FP word orders.  */
      if (FLOAT_WORDS_BIG_ENDIAN)
        word = (GET_MODE_BITSIZE (fmode) - bitpos) / BITS_PER_WORD;
      else
        word = bitpos / BITS_PER_WORD;
      temp = operand_subword_force (temp, word, fmode);
      bitpos = bitpos % BITS_PER_WORD;
    }

rl78 has 8 bit words.  So we get into the else clause  and call
operand_subword_force with

(gdb) p debug_rtx (temp)
(reg/v:SF 45 [ fp ])
$15 = void
(gdb) p word
$16 = 3
(gdb) p fmode
$17 = {m_mode = E_SFmode}

Which generally makes sense. That returns:
(subreg:QI (reg/v:SF 45 [ fp ]) 3)

And bitpos will be set to 7.  ie, check the sign bit, exactly what we
should expect.

We copy the subreg expression into a pseudo:

(reg:QI 46)

Remember that we wanted the result in HImode.  So we then get into this
conditional:

  if (bitpos < GET_MODE_BITSIZE (rmode))
    {
      wide_int mask = wi::set_bit_in_zero (bitpos, GET_MODE_PRECISION
(rmode));

      if (GET_MODE_SIZE (imode) > GET_MODE_SIZE (rmode))
        temp = gen_lowpart (rmode, temp);
      temp = expand_binop (rmode, and_optab, temp,
                           immed_wide_int_const (mask, rmode),
                           NULL_RTX, 1, OPTAB_LIB_WIDEN);
    }

Which again makes perfect sense.  We want to do a bit-wise AND in HImode
of the pseudo and the mask.  So far, so good.


The target doesn't have HImode logicals.  But they can be synthesized
from QImode logicals by this loop in expand_binop:

 /* These can be done a word at a time.  */
  if ((binoptab == and_optab || binoptab == ior_optab || binoptab ==
xor_optab)
      && is_int_mode (mode, &int_mode)
      && GET_MODE_SIZE (int_mode) > UNITS_PER_WORD
      && optab_handler (binoptab, word_mode) != CODE_FOR_nothing)

[ ... ]

      /* Do the actual arithmetic.  */
      for (i = 0; i < GET_MODE_BITSIZE (int_mode) / BITS_PER_WORD; i++)
        {
          rtx target_piece = operand_subword (target, i, 1, int_mode);
          rtx x = expand_binop (word_mode, binoptab,
                                operand_subword_force (op0, i, int_mode),
                                operand_subword_force (op1, i, int_mode),
                                target_piece, unsignedp, next_methods);

          if (x == 0)
            break;

          if (target_piece != x)
            emit_move_insn (target_piece, x);
        }

This is where things start to get interesting.

Notice the calls to operand_subword_force.

We'll end up calling operand_subword_force with:

Breakpoint 4, operand_subword_force (op=0x7fffe9f148a0, offset=...,
mode=E_HImode) at /home/gcc/GIT-3/gcc/gcc/emit-rtl.c:1777
1777      rtx result = operand_subword (op, offset, 1, mode);
(gdb) p debug_rtx (op)
(reg:QI 46)
$28 = void
(gdb) p offset
$29 = {<poly_int_pod<1, unsigned long>> = {coeffs = {0}}, <No data fields>}
(gdb) p mode
$30 = E_HImode


INT_MODE as passed into operand_subword_force is documented as the mode
to use if the first operand is a constant integer (which are modeless).
In theory it shouldn't be used for other operands.  But reality is
different.

operand_subword_force will call operand_subword.  operand_subword will
eventually call simplify_gen_subreg/simplify_subreg.  In that call
OUTERMODE will be WORD_MODE, INNERMODE will be that passed in MODE to
operands_subword_force (HImode in this case).


simplify_subreg is going to hit this assertion:

 gcc_assert (GET_MODE (op) == innermode
              || GET_MODE (op) == VOIDmode);

Remember OP is a QImode register.  INNERMODE is HImode that we passed
along just in case OP was a CONST_INT.  And boom, we ICE.


I've gone back and forth a few times on the best place to fix this.  At
one time I just simplified this special case in operand_subword so that
we'd never have to pass it off to simplify_gen_subreg/simplify_subreg.
But that doesn't seem right.    I keep coming back to those original
calls to operand_subword_force.

Blindly passing in INT_MODE there just seems wrong. ISTM we should only
be passing in INT_MODE when the operand is a constant.  For non-constant
operands, we should just pass in VOIDmode.  In the case where we end up
inside simplify_gen_subreg/simplify_subreg that will result in using the
mode of the operand which is precisely what we want.

And that's precisely what this patch does.  That fixes the ICE and I've
verified the assembly code looks right on the rl78 port.  Furthermore,
newlib will successfully build with this patch.

It's been through a full cycle in my tester.  So it's been through
bootstraps on big/little endian systems, built kernels, built runtimes
(glibc, newlib, libgcc) -- essentially covering nearly all our targets
to varying degrees.

Installing on the trunk.

Jeff
diff mbox series

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 3efd96b570e..be75c6874c8 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2018-11-14  Jeff Law  <law@redhat.com>
+
+	* optabs.c (expand_binop): Pass INT_MODE to operand_subword_force
+	iff the operand is a constant.
+
 2018-11-14  Aldy Hernandez  <aldyh@redhat.com>
 
 	* gimple-ssa-evrp-analyze.c
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 6052222c90c..c7d1f22e7a8 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -1377,12 +1377,14 @@  expand_binop (machine_mode mode, optab binoptab, rtx op0, rtx op1,
       start_sequence ();
 
       /* Do the actual arithmetic.  */
+      enum machine_mode op0_mode = CONSTANT_P (op0) ? int_mode : VOIDmode;
+      enum machine_mode op1_mode = CONSTANT_P (op1) ? int_mode : VOIDmode;
       for (i = 0; i < GET_MODE_BITSIZE (int_mode) / BITS_PER_WORD; i++)
 	{
 	  rtx target_piece = operand_subword (target, i, 1, int_mode);
 	  rtx x = expand_binop (word_mode, binoptab,
-				operand_subword_force (op0, i, int_mode),
-				operand_subword_force (op1, i, int_mode),
+				operand_subword_force (op0, i, op0_mode),
+				operand_subword_force (op1, i, op1_mode),
 				target_piece, unsignedp, next_methods);
 
 	  if (x == 0)
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 50e53f0b196..cee33796cc5 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@ 
+2018-11-14  Jeff Law  <law@redhat.com>
+
+	* gcc.c-torture/compile/20181114.c: New test.
+
 2018-11-14  Richard Biener  <rguenther@suse.de>
 
 	PR middle-end/87985
diff --git a/gcc/testsuite/gcc.c-torture/compile/20181114-1.c b/gcc/testsuite/gcc.c-torture/compile/20181114-1.c
new file mode 100644
index 00000000000..9bcc3992f64
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/compile/20181114-1.c
@@ -0,0 +1,6 @@ 
+int
+_vfprintf_r (double fp)
+{
+  if (__builtin_signbit (fp))
+    return '-';
+}