On Fri, Apr 20, 2012 at 6:16 PM, Xinliang David Li <davi...@google.com> wrote:
> On Fri, Apr 20, 2012 at 4:50 AM, Richard Guenther
> <richard.guent...@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 12:07 PM, Richard Guenther
>> <richard.guent...@gmail.com> wrote:
>>> On Fri, Apr 20, 2012 at 10:41 AM, Richard Guenther
>>> <richard.guent...@gmail.com> wrote:
>>>> On Thu, Apr 19, 2012 at 8:51 PM, Xinliang David Li <davi...@google.com>
>>>> wrote:
>>>>> Compiling the attached test case with -O2 using the trunk compiler,
>>>>> the compiler will ICE. The proposed patch is also attached. The test
>>>>> is under going, but I like to have discussion on the right fix first.
>>>>
>>>> The patch doesn't look correct. There needs to be a VUSE, if not then
>>>> sth else is wrong.
>>>>
>>>> I'm looking into it.
>>>
>>> It is propagate_tree_value_into_stmt not properly updating SSA form. The
>>> following patch fixes that - but it looks bogus that PRE sees
>>>
>>> t_12 = new 1000;
>>>
>>> as equivalent to t_12 = &g.
>>>
>>> Which is a separate issue. I suppose you can make a runtime testcase
>>> out of it and file a bug?
>>
>> Somewhat simplified testcase that still performs the bogus replacement:
>>
>> int g, *gp;
>> int *bar();
>> void foo (int ***p, int** end, int b)
>> {
>> *p = 0;
>> int** pp = *p;
>> for (;;)
>> {
>> pp++;
>> *pp = &g;
>> if (b)
>> {
>> if (b>1)
>> g = 0;
>> int *t = bar ();
>> *pp = t; <--- b
>> }
>> gp = *pp; <--- a
>> }
>> }
>>
>> we note the bogus equivalency when trying to insert *pp from <--- a into
>> its predecessors. The translated expr in the if (b) block is
>> {mem_ref<8B>,pp_1}@.MEM_16 with a leader 't' which looks ok.
>>
>> But then we are trying to insert *pp from <--- b into its predecessors.
>> Not sure why - but I suppose we removed 't' because it is in TMP_GEN
>> but left *pp as EXP_GEN. *pp ends up in ANTIC_IN because
>> clean does not consider *pp = t clobbering it -
>
> Yes, this is what is happening -- I should have made it clearer. The
> *pp from <--a end up in ANTIC_IN of BB(<--b) after translation which
> is incorrect. The clean(..) method is supposed to remove the invalid
> expressions, but the value_dies_in_block_x returns false because the
> no-alias returned.
>
>
>>which is kind of correct - pp
>> points to NULL only, so storing into it cannot possibly alter what *pp points
>> to. Well, all of this is undefined territory, so if not ICEing then this is
>> no
>> longer a bug (not wrong-code - just an interesting side-effect of
>> undefinedness).
>
> Yes -- that is why whatever fix should be ok as the code won't be
> executed at runtime. For the pointer to null case, for some targets,
> the null dress can be legal then the aliaser's behavior will be wrong.
Yes. Those need to enable -fno-delete-null-pointer-checks and the aliaser
will no longer derive pt_null then.
Richard.
> thanks,
>
> David
>
>
>>
>> Richard.
>>
>>> Thanks,
>>> Richard.
>>>
>>>> Richard.
>>>>
>>>>> thanks,
>>>>>
>>>>> David
>>>>>
>>>>> Analysis:
>>>>> -------------
>>>>>
>>>>> Here is what is happening:
>>>>>
>>>>> BB6 : (Successor: BB8)
>>>>> MEM_19 = phi(MEM_24, MEM_25)
>>>>> t_9 = operator new (100)@MEM_19 (--> MEM_26)
>>>>> mem_ref(pp_6, -16).x@MEM_26 = t_9 (-->MEM_27) --> (1)
>>>>>
>>>>> BB8:
>>>>> ...
>>>>> MEM_20 = PHI(MEM_27, MEM_24)
>>>>> ....
>>>>> d.2348_15 = mem_ref(pp_6, -16).x@MEM_20
>>>>>
>>>>>
>>>>> In the loop header BB3 (which dominates BB6 and BB8),
>>>>>
>>>>> BB3:
>>>>> ..
>>>>> pp_31 = phi (pp_6, 0);
>>>>> ...
>>>>> pp_6 = pp_31 + 16
>>>>>
>>>>>
>>>>>
>>>>> In PRE, ANTIC_IN(BB8) includes mem_ref(pp_31,0),x@MEM_20. After phi
>>>>> translate, ANTIC_OUT(BB6) gets mem_ref(pp_31,0).x@MEM_27. However
>>>>> this expression gets propagated into ANTIC_IN(BB6) even though the
>>>>> memory expression is killed by (1). The root cause is that the alias
>>>>> oracle reports that mem_ref(pp_6, -16) and mem_ref(pp_31, 0) are not
>>>>> aliased as their points-to set do not intersect.
>>>>>
>>>>> As as consequence of the bad aliasing, the operator new calls gets
>>>>> transformed into an gimple assignment -- this gimple assignment
>>>>> becomes the defining statement for MEM_26. In a later UD chain walk,
>>>>> the memory chain stops (and triggers the assert) at this new
>>>>> assignment statement because there is no associated vuse for it.
>>>>>
>>>>>
>>>>> Test case
>>>>> --------------
>>>>>
>>>>> The case is synthesized -- the real world examples involve lots of
>>>>> inlining etc.
>>>>>
>>>>>
>>>>> int g, *gp[100];
>>>>> struct V {
>>>>> int* x;
>>>>> int y;
>>>>> };
>>>>>
>>>>> void foo (V **p, V* end, int i)
>>>>> {
>>>>> *p = 0;
>>>>> V* pp = *p;
>>>>> int s = 100;
>>>>> for (; pp < end; )
>>>>> {
>>>>> pp++;
>>>>> (pp-1)->x = &g;
>>>>> if (g)
>>>>> {
>>>>> if (g>10)
>>>>> g++;
>>>>> int *t = (int*) operator new (100);
>>>>> (pp-1)->x = t;
>>>>> }
>>>>> else
>>>>> s--;
>>>>> gp[end-pp] = (pp-1)->x + s;
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> Patch
>>>>> ---------
>>>>>
>>>>>
>>>>> Index: tree-ssa-structalias.c
>>>>> ===================================================================
>>>>> --- tree-ssa-structalias.c (revision 186600)
>>>>> +++ tree-ssa-structalias.c (working copy)
>>>>> @@ -6137,6 +6137,9 @@ pt_solutions_intersect_1 (struct pt_solu
>>>>> if (pt1->anything || pt2->anything)
>>>>> return true;
>>>>>
>>>>> + if (pt1->null && pt2->null)
>>>>> + return true;
>>>>> +
>>>>> /* If either points to unknown global memory and the other points to
>>>>> any global memory they alias. */
>>>>> if ((pt1->nonlocal
