[PATCH] Use narrow mode of constant when expanding widening multiplication

[Jozef Lawrynowicz]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-2019-10-21-Jozef-Lawrynowicz-jozef.l-mittosystems.co.patch
Type: text/x-patch
Size: 1594 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20191021/d3189cb6/attachment.bin>