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,
Richard
