I found problems in my patch. Resetting the BB to NULL will cause
problems in PreOPT emitter. In Raise_whiledo_stmt_to_doloop() defined
in opt_emit.cxx:
1062 preheader->Remove_stmtrep( preheader->Last_stmtrep());
1063
1064 STMTREP *goto_end = loopback->Branch_stmtrep();
1065 if ( goto_end != NULL)
1066 loopback->Remove_stmtrep( goto_end );
These statements are not dead but removed from BB when they are raised
to do_loop.
If the BB is set to NULL, it will trigger assertions in
DU_MANAGER::Collect_BB_id defined in opt_du.cxx:
1649 void
1650 DU_MANAGER::Collect_BB_id(WN_MAP wn_to_cr_map, WN *wn)
1651 {
1652 const OPERATOR opr = WN_operator(wn);
1653 const STMTREP *stmt = (STMTREP*)WN_MAP_Get( wn_to_cr_map, wn );
1654
1655 if (stmt && (WN_operator(wn) != OPR_FUNC_ENTRY ||
1656 WN_operator(wn) != OPR_ALTENTRY)) {
1657 BB_NODE *bb = stmt->Bb();
1658 BB_SUMMARY_INFO *bs = &_bb_summary[bb->Id()];
1659 Set_bb_id(wn, bb->Id());
1660 if (bs->first_stmt == NULL)
1661 bs->first_stmt = wn;
1662 bs->last_stmt = wn;
1663 if (Tracing())
1664 fprintf(TFile, "stmt (map_id %d) in BB%d\n",
WN_map_id(wn), Get_bb_id(wn));
1665 }
So I think Gang's patch is right and mine is wrong.
2011/12/23 Sun Chan <sun.c...@gmail.com>:
> 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
--
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
