Fix ICE in set_lattice_value

On Thu, Jul 19, 2012 at 9:30 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Hmm, the point is of couse to not allow transitions that could form a
>> cycle, which is why the reverse transition is not allowed.
>>
>> Let me have a closer look here.
>
> You can reproduce on your favorite platform by locally copying the system.ads
> from gcc/ada/rts in your build tree and turning ZCX_By_Default to False.

That helps reproducing it.  The issue is that this really is a transition in the
wrong direction.  We iterate

Visiting statement:
D.2928_263 = (sizetype) i.29_262;
which is likely UNDEFINED
Lattice value changed to UNDEFINED.  Adding SSA edges to worklist.
...
Visiting statement:
D.2930_265 = (sizetype) j.30_264;
which is likely CONSTANT
Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.
...
Visiting statement:
elem_266 = &arr[D.2928_263][D.2930_265];
which is likely CONSTANT
Lattice value changed to CONSTANT Lattice value changed to CONSTANT
0x00000000000000000 (0x0fffffffffffffffc).  Adding SSA edges to
worklist.

... 2nd iteration:

Visiting statement:
D.2928_263 = (sizetype) i.29_262;
which is likely CONSTANT
Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.
...
Visiting statement:
elem_266 = &arr[D.2928_263][D.2930_265];
which is likely CONSTANT

so the error is that in the 1st iteration when visiting elem_266 we do not
treat D.2928_263 optimistically enough.  We could either derive a
lattice-value of &arr[0][0] or simply make it UNDEFINED (which sounds
like the easier solution).

So I'm testing the following


Richard.

> --
> Eric Botcazou

> That helps reproducing it.  The issue is that this really is a transition
> in the wrong direction.  We iterate
> 
> Visiting statement:
> D.2928_263 = (sizetype) i.29_262;
> which is likely UNDEFINED
> Lattice value changed to UNDEFINED.  Adding SSA edges to worklist.
> ...
> Visiting statement:
> D.2930_265 = (sizetype) j.30_264;
> which is likely CONSTANT
> Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.
> ...
> Visiting statement:
> elem_266 = &arr[D.2928_263][D.2930_265];
> which is likely CONSTANT
> Lattice value changed to CONSTANT Lattice value changed to CONSTANT
> 0x00000000000000000 (0x0fffffffffffffffc).  Adding SSA edges to
> worklist.
> 
> ... 2nd iteration:
> 
> Visiting statement:
> D.2928_263 = (sizetype) i.29_262;
> which is likely CONSTANT
> Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.
> ...
> Visiting statement:
> elem_266 = &arr[D.2928_263][D.2930_265];
> which is likely CONSTANT
> 
> so the error is that in the 1st iteration when visiting elem_266 we do not
> treat D.2928_263 optimistically enough.  We could either derive a
> lattice-value of &arr[0][0] or simply make it UNDEFINED (which sounds
> like the easier solution).

OK, thanks for the explanation.

> So I'm testing the following
> 
> Index: gcc/tree-ssa-ccp.c
> ===================================================================
> --- gcc/tree-ssa-ccp.c  (revision 189646)
> +++ gcc/tree-ssa-ccp.c  (working copy)
> @@ -648,6 +649,11 @@ likely_value (gimple stmt)
>              the undefined operand may be promoted.  */
>           return UNDEFINED;
> 
> +       case ADDR_EXPR:
> +         /* If any part of an address is UNDEFINED, like the index
> +            of an ARRAY_EXPR, then treat the result as UNDEFINED.  */
> +         return UNDEFINED;
> +
>         default:
>           ;
>         }

I can do the full testing if you're happy with it.

On Thu, Jul 19, 2012 at 12:00 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> That helps reproducing it.  The issue is that this really is a transition
>> in the wrong direction.  We iterate
>>
>> Visiting statement:
>> D.2928_263 = (sizetype) i.29_262;
>> which is likely UNDEFINED
>> Lattice value changed to UNDEFINED.  Adding SSA edges to worklist.
>> ...
>> Visiting statement:
>> D.2930_265 = (sizetype) j.30_264;
>> which is likely CONSTANT
>> Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.
>> ...
>> Visiting statement:
>> elem_266 = &arr[D.2928_263][D.2930_265];
>> which is likely CONSTANT
>> Lattice value changed to CONSTANT Lattice value changed to CONSTANT
>> 0x00000000000000000 (0x0fffffffffffffffc).  Adding SSA edges to
>> worklist.
>>
>> ... 2nd iteration:
>>
>> Visiting statement:
>> D.2928_263 = (sizetype) i.29_262;
>> which is likely CONSTANT
>> Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.
>> ...
>> Visiting statement:
>> elem_266 = &arr[D.2928_263][D.2930_265];
>> which is likely CONSTANT
>>
>> so the error is that in the 1st iteration when visiting elem_266 we do not
>> treat D.2928_263 optimistically enough.  We could either derive a
>> lattice-value of &arr[0][0] or simply make it UNDEFINED (which sounds
>> like the easier solution).
>
> OK, thanks for the explanation.
>
>> So I'm testing the following
>>
>> Index: gcc/tree-ssa-ccp.c
>> ===================================================================
>> --- gcc/tree-ssa-ccp.c  (revision 189646)
>> +++ gcc/tree-ssa-ccp.c  (working copy)
>> @@ -648,6 +649,11 @@ likely_value (gimple stmt)
>>              the undefined operand may be promoted.  */
>>           return UNDEFINED;
>>
>> +       case ADDR_EXPR:
>> +         /* If any part of an address is UNDEFINED, like the index
>> +            of an ARRAY_EXPR, then treat the result as UNDEFINED.  */
>> +         return UNDEFINED;
>> +
>>         default:
>>           ;
>>         }
>
> I can do the full testing if you're happy with it.

I'm running bootstrap & testing on x86_64 right now and will commit the
patch (it's quite sound to me - ADDR_EXPR is no different from PLUS_EXPR
or POINTER_PLUS_EXPR with respect to UNDEFINED handling).

Thanks,
Richard.

> --
> Eric Botcazou

> I'm running bootstrap & testing on x86_64 right now and will commit the
> patch (it's quite sound to me - ADDR_EXPR is no different from PLUS_EXPR
> or POINTER_PLUS_EXPR with respect to UNDEFINED handling).

Thanks.  It would be nice to commit the testcase I posted in the first message.