On Wed, 9 Oct 2019 16:03:34 +0200
Jakub Jelinek <ja...@redhat.com> wrote:
> On Wed, Oct 09, 2019 at 02:40:42PM +0100, Jozef Lawrynowicz wrote:
> > I've added a new define_expand for msp430 to handle "mulhisi", but when
> > testing
> > the changes, some builtin tests (e.g. builtin-arith-overflow-{1,5,p-1}.c)
> > fail.
> >
> > I've narrowed a test case down to:
> >
> > 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))
>
> Yes, that is not valid, it needs to be (const_int -1).
>
> > I guess the bug is wherever the (const_int 65535) is generated, it should
> > be -1
> > sign extend to a HWI. That is based on this statement from the docs:
>
> Yes. You need to debug where it is created ((const_int 65535) itself is not
> wrong if it is e.g. meant for SImode or DImode etc., but when it is to be
> used in HImode context, it needs to be canonicalized) and fix.
>
> Jakub
Thanks for the responses, took me a little while to get back to looking at this.
In the end I tracked this down to some behaviour specific to the mul_overflow
builtins.
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.
I'm currently testing the following patch which fixes the problem:
diff --git a/gcc/expmed.c b/gcc/expmed.c
index f1975fe33fe..5a2516dfc15 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -3748,6 +3748,18 @@ expand_mult_highpart_adjust (scalar_int_mode mode, rtx
adj_operand, rtx op0, rtx tem;
enum rtx_code adj_code = unsignedp ? PLUS : MINUS;
+ /* Constants that have been converted from a mode with
+ prec <= HOST_BITS_PER_WIDE_INT to a wider mode and back again may not be
+ canonically represented. So we check if the high bit is set (which
indicates
+ if the constant might be ambiguously represented), and
+ canonicalize the constant if it is. */
+ if (CONST_INT_P (op0)
+ && (UINTVAL (op0) & (HOST_WIDE_INT_1U << (GET_MODE_BITSIZE (mode) - 1))))
+ op0 = gen_int_mode (INTVAL (op0), mode);
+ if (CONST_INT_P (op1)
+ && (UINTVAL (op1) & (HOST_WIDE_INT_1U << (GET_MODE_BITSIZE (mode) - 1))))
+ op1 = gen_int_mode (INTVAL (op1), mode);
+
tem = expand_shift (RSHIFT_EXPR, mode, op0,
GET_MODE_BITSIZE (mode) - 1, NULL_RTX, 0);
tem = expand_and (mode, tem, op1, NULL_RTX);
I thought about adding this code to expand_binop instead but this seems like
something that should be handled by the caller. Also, we don't have
this problem when expanding any other RTL.
However, we do already have somewhat similar behaviour of shifts in expand_binop
/* For shifts, constant invalid op1 might be expanded from different
mode than MODE. As those are invalid, force them to a register
to avoid further problems during expansion. */
else if (CONST_INT_P (op1)
&& shift_optab_p (binoptab)
&& UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode)))
{
op1 = gen_int_mode (INTVAL (op1), GET_MODE_INNER (mode));
op1 = force_reg (GET_MODE_INNER (mode), op1);
}
For now I'll stick with the fix in expand_mult_highpart_adjust and see how the
tests go.
Jozef
