Use narrow mode of constant when expanding widening multiplication

Message ID 20191018163058.3c6864f0@jozef-kubuntu
State New
Headers show
Series
  • Use narrow mode of constant when expanding widening multiplication
Related show

Commit Message

Jozef Lawrynowicz Oct. 18, 2019, 3:30 p.m.
I experienced the following ICE when working on a downstream patch for msp430:

void
foo (unsigned int r, unsigned int y)
{
  __builtin_umul_overflow ((unsigned int) (-1), y, &r);
}

> msp430-elf-gcc -S tester.c -O0


tester.c: In function 'foo':
tester.c:4:1: error: unrecognizable insn:
    4 | }
      | ^
(insn 16 15 17 2 (set (reg:HI 32)
        (const_int 65535 [0xffff])) "tester.c":3:3 -1
     (nil))
during RTL pass: vregs
dump file: tester.c.234r.vregs
tester.c:4:1: internal compiler error: in extract_insn, at recog.c:2311

Following discussions on ml/gcc
(https://gcc.gnu.org/ml/gcc/2019-10/msg00083.html), I narrowed this down to a
call to expand_mult_highpart_adjust in expand_expr_real_2.

If one of the operands is a constant, its mode had been converted to the wide
mode of our multiplication to generate some RTL, but not converted back to the
narrow mode before expanding what will be the high part of the result of the
multiplication.

If we look at the other two uses of expand_mult_highpart_adjust in the sources,
(both in expmed.c (expmed_mult_highpart_optab)) we can see that the narrow
version of the constant is always used:
      if (tem)
        /* We used the wrong signedness.  Adjust the result.  */
        return expand_mult_highpart_adjust (mode, tem, op0, narrow_op1,
                                            tem, unsignedp);

So the attached patch updates the use in expand_expr_real_2 to also use the
narrow version of the constant operand.
This fixes the aforementioned ICE.

Successfully bootstrapped and regtested for x86_64-pc-linux-gnu.
Successfully regtested for msp430-elf.

Ok for trunk?

Comments

Richard Sandiford Oct. 21, 2019, 7:40 a.m. | #1
Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:
> I experienced the following ICE when working on a downstream patch for msp430:

>

> void

> foo (unsigned int r, unsigned int y)

> {

>   __builtin_umul_overflow ((unsigned int) (-1), y, &r);

> }

>

>> msp430-elf-gcc -S tester.c -O0

>

> tester.c: In function 'foo':

> tester.c:4:1: error: unrecognizable insn:

>     4 | }

>       | ^

> (insn 16 15 17 2 (set (reg:HI 32)

>         (const_int 65535 [0xffff])) "tester.c":3:3 -1

>      (nil))

> during RTL pass: vregs

> dump file: tester.c.234r.vregs

> tester.c:4:1: internal compiler error: in extract_insn, at recog.c:2311

>

> Following discussions on ml/gcc

> (https://gcc.gnu.org/ml/gcc/2019-10/msg00083.html), I narrowed this down to a

> call to expand_mult_highpart_adjust in expand_expr_real_2.

>

> If one of the operands is a constant, its mode had been converted to the wide

> mode of our multiplication to generate some RTL, but not converted back to the

> narrow mode before expanding what will be the high part of the result of the

> multiplication.

>

> If we look at the other two uses of expand_mult_highpart_adjust in the sources,

> (both in expmed.c (expmed_mult_highpart_optab)) we can see that the narrow

> version of the constant is always used:

>       if (tem)

>         /* We used the wrong signedness.  Adjust the result.  */

>         return expand_mult_highpart_adjust (mode, tem, op0, narrow_op1,

>                                             tem, unsignedp);

>

> So the attached patch updates the use in expand_expr_real_2 to also use the

> narrow version of the constant operand.

> This fixes the aforementioned ICE.


TBH, I don't understand why we have:

		  if (TREE_CODE (treeop1) == INTEGER_CST)
		    op1 = convert_modes (mode, word_mode, op1,
					 TYPE_UNSIGNED (TREE_TYPE (treeop1)));

All the following code treats op1 as having the original mode (word_mode),
and having the same mode as op0.  That's what both the optab and (like you
say) expand_mult_highpart_adjust expect.

So unless I'm missing something, it looks like all the code does is
introduce exactly this bug.

AFAICT from the history, having separate code for INTEGER_CST dates back
to when this was an expand peephole for normal MULT_EXPRs.  In that case
we had to handle two cases: when op1 was a conversion from a narrower type,
and when it was an INTEGER_CST with a suitable range.  The separate
INTEGER_CST case then got carried along in further updates but I think
became redundant when the code was moved to WIDEN_MULT_EXPR.

Removing the above is pre-approved if it works.

Thanks,
Richard
Jozef Lawrynowicz Oct. 21, 2019, 8:51 p.m. | #2
On Mon, 21 Oct 2019 08:40:11 +0100
Richard Sandiford <richard.sandiford@arm.com> wrote:

> Jozef Lawrynowicz <jozef.l@mittosystems.com> writes:

> > I experienced the following ICE when working on a downstream patch for msp430:

> >

> > void

> > foo (unsigned int r, unsigned int y)

> > {

> >   __builtin_umul_overflow ((unsigned int) (-1), y, &r);

> > }

> >  

> >> msp430-elf-gcc -S tester.c -O0  

> >

> > tester.c: In function 'foo':

> > tester.c:4:1: error: unrecognizable insn:

> >     4 | }

> >       | ^

> > (insn 16 15 17 2 (set (reg:HI 32)

> >         (const_int 65535 [0xffff])) "tester.c":3:3 -1

> >      (nil))

> > during RTL pass: vregs

> > dump file: tester.c.234r.vregs

> > tester.c:4:1: internal compiler error: in extract_insn, at recog.c:2311

> >

> > Following discussions on ml/gcc

> > (https://gcc.gnu.org/ml/gcc/2019-10/msg00083.html), I narrowed this down to a

> > call to expand_mult_highpart_adjust in expand_expr_real_2.

> >

> > If one of the operands is a constant, its mode had been converted to the wide

> > mode of our multiplication to generate some RTL, but not converted back to the

> > narrow mode before expanding what will be the high part of the result of the

> > multiplication.

> >

> > If we look at the other two uses of expand_mult_highpart_adjust in the sources,

> > (both in expmed.c (expmed_mult_highpart_optab)) we can see that the narrow

> > version of the constant is always used:

> >       if (tem)

> >         /* We used the wrong signedness.  Adjust the result.  */

> >         return expand_mult_highpart_adjust (mode, tem, op0, narrow_op1,

> >                                             tem, unsignedp);

> >

> > So the attached patch updates the use in expand_expr_real_2 to also use the

> > narrow version of the constant operand.

> > This fixes the aforementioned ICE.  

> 

> TBH, I don't understand why we have:

> 

> 		  if (TREE_CODE (treeop1) == INTEGER_CST)

> 		    op1 = convert_modes (mode, word_mode, op1,

> 					 TYPE_UNSIGNED (TREE_TYPE (treeop1)));

> 

> All the following code treats op1 as having the original mode (word_mode),

> and having the same mode as op0.  That's what both the optab and (like you

> say) expand_mult_highpart_adjust expect.

> 

> So unless I'm missing something, it looks like all the code does is

> introduce exactly this bug.

> 

> AFAICT from the history, having separate code for INTEGER_CST dates back

> to when this was an expand peephole for normal MULT_EXPRs.  In that case

> we had to handle two cases: when op1 was a conversion from a narrower type,

> and when it was an INTEGER_CST with a suitable range.  The separate

> INTEGER_CST case then got carried along in further updates but I think

> became redundant when the code was moved to WIDEN_MULT_EXPR.

> 

> Removing the above is pre-approved if it works.


Thanks for providing that background info. I'm happy to just remove that code as
you suggest, since it also fixes the ICE.

Applied the attached patch.
Bootstrapped and regtested on trunk for x86_64-pc-linux-gnu.
Regtested on trunk for msp430-elf.

Jozef

> 

> Thanks,

> Richard
From ad651a1e1fcd0c508c43088a9480d7122dafebde Mon Sep 17 00:00:00 2001
From: jozefl <jozefl@138bc75d-0d04-0410-961f-82ee72b054a4>

Date: Mon, 21 Oct 2019 20:44:16 +0000
Subject: [PATCH] 2019-10-21  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* expr.c (expand_expr_real_2): Don't widen constant op1 when expanding
	widening multiplication.


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@277271 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/ChangeLog | 5 +++++
 gcc/expr.c    | 3 ---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 50121777940..2aa5536cf32 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@
+2019-10-21  Jozef Lawrynowicz  <jozef.l@mittosystems.com>
+
+	* expr.c (expand_expr_real_2): Don't widen constant op1 when expanding
+	widening multiplication.
+
 2019-10-21  Richard Earnshaw  <rearnsha@arm.com>
 
 	* config/arm/iterators.md (t2_binop0): Fix typo in comment.
diff --git a/gcc/expr.c b/gcc/expr.c
index b54bf1d3dc5..476c6865f20 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -8954,9 +8954,6 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
 		     != INTEGER_CST check.  Handle it.  */
 		  if (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode)
 		    goto widen_mult_const;
-		  if (TREE_CODE (treeop1) == INTEGER_CST)
-		    op1 = convert_modes (mode, word_mode, op1,
-					 TYPE_UNSIGNED (TREE_TYPE (treeop1)));
 		  temp = expand_binop (mode, other_optab, op0, op1, target,
 				       unsignedp, OPTAB_LIB_WIDEN);
 		  hipart = gen_highpart (word_mode, temp);
-- 
2.17.1

Patch

From b430cddbd257353f162fe3968a447b63cbcaa964 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Thu, 17 Oct 2019 18:22:01 +0100
Subject: [PATCH] Use narrow mode of constant when expanding widening
 multiplication

gcc/ChangeLog:

2019-10-18  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	* expr.c (expand_expr_real_2): Use op1 in its original narrow mode when
	calling expand_mult_highpart_adjust. 

---
 gcc/expr.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/expr.c b/gcc/expr.c
index b54bf1d3dc5..0a571d3f7e3 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -8947,9 +8947,12 @@  expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
 		  != CODE_FOR_nothing
 		  && innermode == word_mode)
 		{
-		  rtx htem, hipart;
+		  rtx htem, hipart, narrow_op1;
 		  op0 = expand_normal (treeop0);
 		  op1 = expand_normal (treeop1);
+		  /* Save op1 in the narrower mode WORD_MODE for when we expand
+		     the high part.  */
+		  narrow_op1 = op1;
 		  /* op0 and op1 might be constants, despite the above
 		     != INTEGER_CST check.  Handle it.  */
 		  if (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode)
@@ -8961,7 +8964,7 @@  expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
 				       unsignedp, OPTAB_LIB_WIDEN);
 		  hipart = gen_highpart (word_mode, temp);
 		  htem = expand_mult_highpart_adjust (word_mode, hipart,
-						      op0, op1, hipart,
+						      op0, narrow_op1, hipart,
 						      zextend_p);
 		  if (htem != hipart)
 		    emit_move_insn (hipart, htem);
-- 
2.17.1