On Wed, 16 Oct 2019 19:02:17 +0200
Jakub Jelinek <[email protected]> wrote:
> On Wed, Oct 16, 2019 at 05:51:11PM +0100, Jozef Lawrynowicz wrote:
> > We call expand_expr_real_2 from expand_mul_overflow (internal-fn.c:1604).
> > When we process the arguments to:
> > __builtin_umul_overflow ((unsigned int) (-1), y, &r);
> > at expr.c:8952, they go through a few transformations.
> >
> > First we generate the rtx for ((unsigned int) -1) in the HImode context
> > (msp430
> > has 16-bit int), which generates (const_int -1). OK.
> > Then it gets widened in a SImode context, but since it is unsigned, we zero
> > extend and the rtx becomes (const_int 65535). OK.
> > When we call expand_mult_highpart_adjust, we are back in HImode, but using
> > operands which have been widened in a SImode context. This is when we
> > generate our problematic insns using (const_int 65535) with HImode
> > operands.
>
> So, what exactly calls expand_mult_highpart_adjust, with what exact
> arguments (I see 3 callers).
> E.g. the one in expr.c already has:
> if (TREE_CODE (treeop1) == INTEGER_CST)
> op1 = convert_modes (mode, word_mode, op1,
> TYPE_UNSIGNED (TREE_TYPE (treeop1)));
> and should thus take care of op1. It doesn't have the same for op0, assumes
> that if only one operand is INTEGER_CST, it must be the (canonical) second
> one. So perhaps the bug is that something doesn't canonicalize the order of
> arguments?
>
> Jakub
That convert_modes call is actually what changes op1 from (const_int -1) to
(const_int 65535). In expand_expr_real_2, mode is SImode and word_mode is HImode
for that call.
Info from GDB:
> Breakpoint 2, expand_mult_highpart_adjust (<snip>
> unsignedp=unsignedp@entry=1) at ../../gcc/expmed.c:3747
> op0 == (reg:HI 23 [ y.0_1 ])
> op1 == (const_int 65535 [0xffff])
> mode == {m_mode = E_HImode}
> (gdb) bt
> #0 expand_mult_highpart_adjust (<snip> unsignedp=unsignedp@entry=1) at
> ../../gcc/expmed.c:3747
> #1 0x000000000085ee18 in expand_expr_real_2 (<snip>
> tmode=tmode@entry=E_SImode, modifier=modifier@entry=EXPAND_NORMAL) at
> ../../gcc/expr.c:8963
> #2 0x000000000098d01d in expand_mul_overflow (<snip>) at
> ../../gcc/internal-fn.c:1604
> #3 0x000000000098fe2d in expand_arith_overflow (code=MULT_EXPR,
> stmt=<optimised out>) at ../../gcc/internal-fn.c:2362
from expr.c:8946:
if (find_widening_optab_handler (other_optab, mode, innermode)
!= CODE_FOR_nothing
&& innermode == word_mode)
{
rtx htem, hipart;
op0 = expand_normal (treeop0);
***** Below generates (const_int -1) *******
op1 = expand_normal (treeop1);
/* op0 and op1 might be constants, despite the above
!= INTEGER_CST check. Handle it. */
if (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode)
goto widen_mult_const;
if (TREE_CODE (treeop1) == INTEGER_CST)
***** Below generates (const_int 65535) ******
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);
htem = expand_mult_highpart_adjust (word_mode, hipart,
op0, op1, hipart,
zextend_p);
Maybe the constants should be canonicalized before calling
expand_mult_high_part_adjust? I'm not sure at the moment.
Below patch is an alternative I quickly tried that also fixes the issue, but I
haven't tested it and its not clear if op0 should also be converted.
diff --git a/gcc/expmed.c b/gcc/expmed.c
index f1975fe33fe..25d8edde02e 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -3748,6 +3748,8 @@ expand_mult_highpart_adjust (scalar_int_mode mode, rtx
adj_operand, rtx op0,
rtx tem;
enum rtx_code adj_code = unsignedp ? PLUS : MINUS;
+ op1 = convert_modes (mode, GET_MODE (XEXP (adj_operand, 0)), op1, unsignedp);
+
tem = expand_shift (RSHIFT_EXPR, mode, op0,
GET_MODE_BITSIZE (mode) - 1, NULL_RTX, 0);
tem = expand_and (mode, tem, op1, NULL_RTX);
Thanks,
Jozef
