OK. I am ok with the dce fix. The opt_bb.cxx is a new file that I
don't have a lot of experience with and I assume your change is fine.
Anyone else can help review opt_bb.cxx?
Sun
On Fri, Dec 23, 2011 at 3:23 PM, Jian-Xin Lai <laij...@gmail.com> wrote:
> The reason to remove "Reset_live_stmt":
> This line is added when I fix an issue in WHIRL SSA. That issue is
> also fixed by an extra rename pass in WHIRL SSA later. So this change
> looks like unnecessary.
>
> The reason to add Set_bb(NULL):
> Set_bb(NULL) is used to distinguish the real dead code(already removed
> from the BB) and unvisited code (marked to dead at the beginning of
> DCE).
>
> 2011/12/23 Sun Chan <sun.c...@gmail.com>:
>> Asking the third time, why remove Reset_live_stmt() and add Set_bb(null)?
>> Sun
>>
>> On Fri, Dec 23, 2011 at 2:29 PM, Jian-Xin Lai <laij...@gmail.com> wrote:
>>> The line is this one:
>>> stmt->Reset_live_stmt(); // WHIRL SSA: mark stmt dead
>>> This line is introduced by WHIRL SSA. The rename pass is in WHIRL SSA,
>>> not in HSSA. It's different problem.
>>>
>>> 2011/12/23 Sun Chan <sun.c...@gmail.com>:
>>>> I am still confused. You said you "added this line when we hit a
>>>> bug...", which line? I also recall Fred has issue with you folks
>>>> adding an extra rename pass.
>>>> My question, that you have not address, is why add this Set_bb(NULL)
>>>> and remove the line
>>>> Reset_live_stmt()?
>>>> Sun
>>>>
>>>> On Fri, Dec 23, 2011 at 2:17 PM, Jian-Xin Lai <laij...@gmail.com> wrote:
>>>>> Set_bb(NULL) is used to distinguish the real dead code(already removed
>>>>> from the BB) and unvisited code (marked to dead at the beginning of
>>>>> DCE). An alternative approach is to introduce a new flag for real dead
>>>>> code but it requires more changes. Since the statement has been
>>>>> removed from the BB, I think it's OK to set the BB to NULL.
>>>>>
>>>>> I recalled the reason why I added this line when we hit a bug in WHIRL
>>>>> SSA. It's also caused by the similar case that an opnd of phi comes
>>>>> from a chi associated to a dead statement and the version of that phi
>>>>> opnd is wrong. We added an extra rename pass to WHIRL SSA to correct
>>>>> the version. With the extra rename pass, this change becomes
>>>>> unnecessary. (This rename pass is necessary because the WHIRL CFG is
>>>>> not identical to HSSA CFG and the rename pass is needed to correct the
>>>>> order of phi opnds.)
>>>>>
>>>>> 2011/12/22 Sun Chan <sun.c...@gmail.com>:
>>>>>> can you go over why the second change, set_BB(null)? And remove the
>>>>>> mark Reset_live_stmt()?
>>>>>> Sun
>>>>>>
>>>>>> On Thu, Dec 22, 2011 at 3:54 PM, Jian-Xin Lai <laij...@gmail.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Here is a smaller case for bug #897 and a new patch based on Gang's
>>>>>>> current work. Could a gatekeeper review it? Thank you very much.
>>>>>>>
>>>>>>> A smaller case:
>>>>>>> 1 int g_1;
>>>>>>> 2 int* g_2;
>>>>>>> 3 int* func(int* p_1, short p_2) {
>>>>>>> 4 int **l_1;
>>>>>>> 5 const unsigned long l_2 = 0x2A1DFCF9L;
>>>>>>> 6 for ( ; ; p_2 = foo() ) {
>>>>>>> 7 int ***l_3 = &l_1;
>>>>>>> 8 (*l_3) = &g_2;
>>>>>>> 9 if (!l_2) {
>>>>>>> 10 // The following code is unreachable
>>>>>>> 11 if ((bar())) {
>>>>>>> 12 (*g_2) |= g_1;
>>>>>>> 13 (**l_1) |= (*p_1);
>>>>>>> 14 g_2 = &g_1;
>>>>>>> 15 }
>>>>>>> 16 }
>>>>>>> 17 }
>>>>>>> 18 }
>>>>>>>
>>>>>>> in DCE phase, the unreachable code elimination runs at first and all
>>>>>>> the statements in the if block is removed and set to NOT_LIVE. In the
>>>>>>> later Dead store elimination phase, as Gang's explaination, we follow
>>>>>>> the U-D chain of g_2, which is a global variable and has CHI on line
>>>>>>> 6(foo), 11(bar), 12 (*g_2) and a real dec in line 14. When we mark the
>>>>>>> call of foo (line 6) to be LIVE, the CHI of g_2 is also LIVE and
>>>>>>> following the U-D chain, the CHI's on bar() and (*g_2) is also marked
>>>>>>> to LIVE. When marking (*g_2) to live, we also mark that BB to live.
>>>>>>> But the BB is empty (all statements have been removed in earlier
>>>>>>> phase) and the assertion occures.
>>>>>>>
>>>>>>> Here is a new patch:
>>>>>>> Index: osprey/be/opt/opt_dce.cxx
>>>>>>> ===================================================================
>>>>>>> --- osprey/be/opt/opt_dce.cxx (revision 3855)
>>>>>>> +++ osprey/be/opt/opt_dce.cxx (working copy)
>>>>>>> @@ -2619,7 +2619,10 @@
>>>>>>> // NOTE: May at some time always have a defining statement, so
>>>>>>> // we could remove the check for a null defstmt
>>>>>>> // (i.e., if live-in values get some dummy def stmt)
>>>>>>> - if ( cr->Defstmt() != NULL && !cr->Defstmt()->Live_stmt() ) {
>>>>>>> + // if defining statement doesn't belong to any BB, it's been
>>>>>>> + // removed in early unreachable code elimination phase.
>>>>>>> + if ( cr->Defstmt() != NULL && !cr->Defstmt()->Live_stmt() &&
>>>>>>> + cr->Defstmt()->Bb() != NULL ) {
>>>>>>> // if we're making this statement live just because it's
>>>>>>> // associated with a chi, see if we can get rid of "i = i"
>>>>>>> // assignments. This only happens when zero-version is
>>>>>>> Index: osprey/be/opt/opt_bb.cxx
>>>>>>> ===================================================================
>>>>>>> --- osprey/be/opt/opt_bb.cxx (revision 3855)
>>>>>>> +++ osprey/be/opt/opt_bb.cxx (working copy)
>>>>>>> @@ -734,6 +734,7 @@
>>>>>>>
>>>>>>> _stmtlist.Remove(stmt);
>>>>>>> - stmt->Reset_live_stmt(); // WHIRL SSA: mark stmt dead
>>>>>>> + stmt->Set_bb(NULL);
>>>>>>> }
>>>>>>>
>>>>>>> void
>>>>>>>
>>>>>>> If the statement has been removed from the BB, we reset its BB to
>>>>>>> NULL. In the later phase, if a statement's BB is NULL, which means the
>>>>>>> statement has been removed and we don't need to mark it to live.
>>>>>>> Reseting the bb of dead statement to NULL also helps preventing more
>>>>>>> bugs when trying to traverse the BB.
>>>>>>>
>>>>>>> --
>>>>>>> Regards,
>>>>>>> Lai Jian-Xin
>>>>>>>
>>>>>>> ------------------------------------------------------------------------------
>>>>>>> Write once. Port to many.
>>>>>>> Get the SDK and tools to simplify cross-platform app development. Create
>>>>>>> new or port existing apps to sell to consumers worldwide. Explore the
>>>>>>> Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
>>>>>>> http://p.sf.net/sfu/intel-appdev
>>>>>>> _______________________________________________
>>>>>>> Open64-devel mailing list
>>>>>>> Open64-devel@lists.sourceforge.net
>>>>>>> https://lists.sourceforge.net/lists/listinfo/open64-devel
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Regards,
>>>>> Lai Jian-Xin
>>>
>>>
>>>
>>> --
>>> Regards,
>>> Lai Jian-Xin
>
>
>
> --
> Regards,
> Lai Jian-Xin
------------------------------------------------------------------------------
Write once. Port to many.
Get the SDK and tools to simplify cross-platform app development. Create
new or port existing apps to sell to consumers worldwide. Explore the
Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join
http://p.sf.net/sfu/intel-appdev
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
