Re: [patch] convert.c: Fix PR tree-optimization/25125. (Take 2)

[Eric Botcazou <ebotcazou at adacore dot com>]

> On Tue, 20 Dec 2005, Kazu Hirata wrote:
> > 1. convert.c:convert_to_integer changes the type of the addition in
> >    "b = ...;" from signed int to signed short.  That is, we now have
> >    b = a + -32768, where '+' is done in signed short.
>
> Just out of curiosity, have you tried using lang_hooks.types.unsigned_type
> to truncate the signed int addition to an *unsigned* int addition.  One
> way of specifying in the middle-end that the results of operations should
> have defined overflow is to use unsigned rather than signed operations.

I think that using lang_hooks.types.unsigned_type trades signed-overflow 
undefinedness in the original case for signed-overflow undefinedness in a 
very similar case.

Kazu's change has introduced regressions in Ada.  A testcase is:

  package p is

      subtype Bitmap_Size_T is Natural range 0 .. 5;

      type Bitmap_Array_T is array (Bitmap_Size_T range <>) of Boolean;
      pragma Pack (Bitmap_Array_T);

      type Bitmap_T (Size : Bitmap_Size_T) is record
	 C : Bitmap_Array_T (1 .. Size);
      end record;

      Empty_Bitmap : Bitmap_T :=  (Size => 0, C => (1..0 => False));

   end;

p.ads:12:03: warning: Storage_Error will be raised at run-time


The Storage_Error is bogus and stems from the TREE_OVERFLOW bit now being set 
on the signed constant -1 in a calculation of the form:

         NOP(int)
           |
       MINUS(uint)
       /        \
CONVERT(uint)  CONVERT(uint) 
      |            |
   (int64)0   (int64)1

Before Kazu's change, the calculation simply was:

       MINUS(int)
       /        \
CONVERT(int) CONVERT(int) 
      |          |
   (int64)0  (int64)1

and no TREE_OVERFLOW bit is set.  In other words (int)(unsigned int)(int64 -1) 
is not equivalent to (int -1) because TREE_OVERFLOW will be set on the result 
of the former.


We didn't manage to find a C testcase failing at runtime, but using a slightly 
tweaked version of Kazu's testcase:

extern void exit (int);
extern void abort (void);
extern unsigned short f (short a) __attribute__((always_inline));

unsigned short
f (short a)
{
  short b;

  if (a > 0)
    return 0;
  b = ((int) a) + - (int) 1;
  return b;
}

int
main (void)
{
  if (sizeof (short) < 2
      || sizeof (short) >= sizeof (int))
    exit (0);

  if (f (0) != 65535)
    abort ();

  exit (0);
}


We have in t30.eustores:

<bb 0>:
  a_2 = 0;
  if (a_2 > 0) goto <L2>; else goto <L3>;

<L2>:;
  D.1344_10 = 0;
  goto <bb 3> (<L4>);

<L3>:;
  a.0_5 = (short unsigned int) a_2;
  D.1349_6 = a.0_5 + 65535;
  b_7 = (short int) D.1349_6;
  b.1_8 = (short unsigned int) b_7;
  D.1344_9 = (int) b.1_8;


and in t31.cpp:

<bb 0>:
  a_2 = 0;
  a.0_5 = 0;
  D.1349_6 = 65535;
  b_7 = -000000001;
  b.1_8 = 00000ffff;
  D.1344_9 = 00000ffff;


Now the leading 0s for b_7's constant mean that it has the TREE_OVERFLOW bit 
set (dump_generic_node discriminates based on host_integerp):

(gdb) step
evaluate_stmt (stmt=0x556b0948)
    at /home/eric/gnat/gnat5_41/src/gcc/tree-ssa-ccp.c:1121
1121      if (simplified && is_gimple_min_invariant (simplified))
(gdb) p debug_tree(stmt)
 <modify_expr 0x556b0948
    type <integer_type 0x556b91cc short int sizes-gimplified public HI
        size <integer_cst 0x556a9318 constant invariant 16>
        unit size <integer_cst 0x556a9330 constant invariant 2>
        align 16 symtab 0 alias set 2 precision 16 min <integer_cst 0x556a92b8 
-32768> max <integer_cst 0x556a92e8 32767>>
    side-effects
    arg 0 <ssa_name 0x55736a28 type <integer_type 0x556b91cc short int>
        visited var <var_decl 0x556b5790 b> def_stmt <modify_expr 0x556b0948>
        version 7>
    arg 1 <nop_expr 0x556aed60 type <integer_type 0x556b91cc short int>

        arg 0 <ssa_name 0x557369f4 type <integer_type 0x556b9228 short 
unsignedint>
            visited var <var_decl 0x55739058 D.1345> def_stmt <modify_expr 
0x556b0900>
            version 6>>
    ../../pr25125.c:12>

(gdb) p debug_tree(simplified)
 <integer_cst 0x55730b40 type <integer_type 0x556b91cc short int> constant 
invariant public static overflow -1>


Without Kazu's patch:

<bb 0>:
  a_2 = 0;
  if (a_2 > 0) goto <L2>; else goto <L3>;

<L2>:;
  D.1340_8 = 0;
  goto <bb 3> (<L4>);

<L3>:;
  b_5 = a_2 + -1;
  b.0_6 = (short unsigned int) b_5;
  D.1340_7 = (int) b.0_6;


<bb 0>:
  a_2 = 0;
  b_5 = -1;
  b.0_6 = 65535;
  D.1340_7 = 65535;


> If using unsigned types, can't be made to work, then I agree that your
> proposal to use flag_wrapv is the next best thing.

Indeed, I think this is the correct fix.

-- 
Eric Botcazou