general_operand not validating "(const_int 65535 [0xffff])"

general_operand not validating "(const_int 65535 [0xffff])"

[Jozef Lawrynowicz]

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))
during RTL pass: vregs
dump file: tester.c.234r.vregs
tester.c:4:1: internal compiler error: in extract_insn, at recog.c:2311

This is clearly a very simple instruction that should be handled by "movhi":

(define_insn "movhi"                                                
  [(set (match_operand:HI 0 "msp_nonimmediate_operand" "=r,rYs,rm") 
        (match_operand:HI 1 "msp_general_operand" "N,riYs,rmi"))]

"msp_general_operand" is an ior of general_operand and a volatile memory
operand.

When debugging this I noticed that "general_operand" is returning 0 for
(const_int 65535 [0xffff]), i.e. it has not validated this operand:

> Breakpoint 3, general_operand (op=op@entry=0x7ffff7309740, mode=mode@entry=E_HImode) at ../../gcc/recog.c:959 959     {
> (gdb) call debug_rtx(op)
> (const_int 65535 [0xffff])

> Run till exit from #0  0x0000000000b56da4 in general_operand (op=op@entry=0x7ffff7309740, mode=<optimised out>, mode@entry=E_HImode) at ../../gcc/recog.c:974
> 0x0000000001194400 in msp430_general_operand (op=0x7ffff7309740, mode=mode@entry=E_HImode) at ../../gcc/config/msp430/predicates.md:37
> 37      ; general_operand refuses to match volatile memory refs.
> Value returned is $3 = 0   

trunc_int_for_mode changes 65535 to -1, which I think is the cause of the
problem:

> Run till exit from #0  trunc_int_for_mode (c=65535, mode=mode@entry=E_HImode) at ../../gcc/explow.c:55
> 0x0000000000b56da4 in general_operand (op=op@entry=0x7ffff7309740, mode=<optimised out>, mode@entry=E_HImode) at ../../gcc/recog.c:974
> 974           && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op))
> Value returned is $2 = -1

So this results in the following clause evaluating to true and general_operand
returning 0:

if (CONST_INT_P (op)                                          
    && mode != VOIDmode                                       
    && trunc_int_for_mode (INTVAL (op), mode) != INTVAL (op)) 
  return 0;                                                   

I haven't narrowed down where the (const_int 65535) is being generated. I
suspect if MODE was VOIDmode we wouldn't have this problem.
I've noticed that normally a constant like would normally show as (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:

Constants generated for modes with fewer bits than in HOST_WIDE_INT must be
sign extended to full width (e.g., with gen_int_mode). For constants for modes
with more bits than in HOST_WIDE_INT the implied high order bits of that con-
stant are copies of the top bit. Note however that values are neither inherently
signed nor inherently unsigned; where necessary, signedness is determined by
the rtl operation instead.

Can anyone offer any further insight? Do I just need to track down what is
generating this const_int and fix that?

Could it be some edge-case interaction because it is an argument to a builtin?

Thanks,
Jozef
 

Re: general_operand not validating "(const_int 65535 [0xffff])"

[Julian Brown]

On Wed, 9 Oct 2019 14:40:42 +0100
Jozef Lawrynowicz <jozef.l@mittosystems.com> wrote:

> Constants generated for modes with fewer bits than in HOST_WIDE_INT
> must be sign extended to full width (e.g., with gen_int_mode). For
> constants for modes with more bits than in HOST_WIDE_INT the implied
> high order bits of that con- stant are copies of the top bit. Note
> however that values are neither inherently signed nor inherently
> unsigned; where necessary, signedness is determined by the rtl
> operation instead.
> 
> Can anyone offer any further insight? Do I just need to track down
> what is generating this const_int and fix that?

I think you need to change a GEN_INT(x) into a gen_int_mode(x, HImode),
somewhere.

HTH,

Julian

Re: general_operand not validating "(const_int 65535 [0xffff])"

[Jakub Jelinek]

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

Re: general_operand not validating "(const_int 65535 [0xffff])"

[Jozef Lawrynowicz]

On Wed, 9 Oct 2019 16:03:34 +0200
Jakub Jelinek <jakub@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

Re: general_operand not validating "(const_int 65535 [0xffff])"

[Jakub Jelinek]

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

Re: general_operand not validating "(const_int 65535 [0xffff])"

[Jozef Lawrynowicz]

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