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

Reply via email to