On Mon, 21 Oct 2019 08:40:11 +0100
Richard Sandiford <richard.sandif...@arm.com> wrote:
> Jozef Lawrynowicz <joze...@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 <joze...@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 <joze...@mittosystems.com>
+
+ * expr.c (expand_expr_real_2): Don't widen constant op1 when expanding
+ widening multiplication.
+
2019-10-21 Richard Earnshaw <rearn...@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
