On Wed, 16 Oct 2019 19:02:17 +0200 Jakub Jelinek <ja...@redhat.com> 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