This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Patch to fix compiler ICE due to bad PRE
On Fri, Apr 20, 2012 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Fri, Apr 20, 2012 at 4:50 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 12:07 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, Apr 20, 2012 at 10:41 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Apr 19, 2012 at 8:51 PM, Xinliang David Li <davidxl@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