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

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

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 5726 bytes --]



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


[-- Attachment #2: Q --]
[-- Type: text/plain, Size: 1906 bytes --]

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 '-';
+}

[PATCH] Fix expand_binop (PR middle-end/88032)

[Jakub Jelinek]

Hi!

On Wed, Nov 14, 2018 at 09:35:30AM -0700, Jeff Law wrote:
> +	* optabs.c (expand_binop): Pass INT_MODE to operand_subword_force
> +	iff the operand is a constant.

This broke gcc.target/i386/pr80173.c testcase.  The problem is
that while operand_subword handles VOIDmode last argument just fine
by using GET_MODE (op), so it is only important to use non-VOIDmode if
op has VOIDmode.  But, operand_subword_force actually has a different
behavior, if mode is VOIDmode (or BLKmode), it acts just as operand_subword
followed by assertion that it succeeded, rather than by trying to deal with
failed operand_subword by forcing it into a pseudo.

In the testcase, op is a hard register, on which operand_subword fails, but
if it is forced into pseudo, it succeeds.

The following patch arranges it by never passing VOIDmode to
operand_subword_force, pass int_mode as previously if opN has VOIDmode, but
instead of passing VOIDmode otherwise pass the actual mode of the opN
operand.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-11-16  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/88032
	* optabs.c (expand_binop): For op0_mode use GET_MODE (op0), unless it
	is VOIDmode, in which case use int_mode.  Similarly for op1_mode.

--- gcc/optabs.c.jj	2018-11-14 17:42:53.044049213 +0100
+++ gcc/optabs.c	2018-11-15 15:45:35.949378049 +0100
@@ -1377,8 +1377,12 @@ expand_binop (machine_mode mode, optab b
       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;
+      enum machine_mode op0_mode = GET_MODE (op0);
+      enum machine_mode op1_mode = GET_MODE (op1);
+      if (op0_mode == VOIDmode)
+	op0_mode = int_mode;
+      if (op1_mode == VOIDmode)
+	op1_mode = int_mode;
       for (i = 0; i < GET_MODE_BITSIZE (int_mode) / BITS_PER_WORD; i++)
 	{
 	  rtx target_piece = operand_subword (target, i, 1, int_mode);


	Jakub

Re: [PATCH] Fix expand_binop (PR middle-end/88032)

[Jeff Law]

On 11/16/18 1:49 AM, Jakub Jelinek wrote:
> Hi!
> 
> On Wed, Nov 14, 2018 at 09:35:30AM -0700, Jeff Law wrote:
>> +	* optabs.c (expand_binop): Pass INT_MODE to operand_subword_force
>> +	iff the operand is a constant.
> 
> This broke gcc.target/i386/pr80173.c testcase.  The problem is
> that while operand_subword handles VOIDmode last argument just fine
> by using GET_MODE (op), so it is only important to use non-VOIDmode if
> op has VOIDmode.  But, operand_subword_force actually has a different
> behavior, if mode is VOIDmode (or BLKmode), it acts just as operand_subword
> followed by assertion that it succeeded, rather than by trying to deal with
> failed operand_subword by forcing it into a pseudo.
> 
> In the testcase, op is a hard register, on which operand_subword fails, but
> if it is forced into pseudo, it succeeds.
> 
> The following patch arranges it by never passing VOIDmode to
> operand_subword_force, pass int_mode as previously if opN has VOIDmode, but
> instead of passing VOIDmode otherwise pass the actual mode of the opN
> operand.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2018-11-16  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/88032
> 	* optabs.c (expand_binop): For op0_mode use GET_MODE (op0), unless it
> 	is VOIDmode, in which case use int_mode.  Similarly for op1_mode.
Yea, that's fine too -- I had this variant in my tree until the last
cycle of testing where I changed it to VOIDmode :-)  Sorry for the breakage.

jeff