Thanks Min.
Since this is not criticial bug fixing, I plan to check in after 5,0
release.
Regards
Shi Hui
On Sat, Oct 29, 2011 at 2:06 AM, Min Zhao <mzhao...@gmail.com> wrote:
> Your new patch looks good to me. Please go ahead.
>
> Thanks,
>
> Min
>
> On Wed, Oct 26, 2011 at 9:40 PM, Hui Shi <kalin....@gmail.com> wrote:
> > Hi Min,
> >
> > Thanks for your review comments. Comments below.
> > I made some minor updated and attach a new patch.
> > Please help check.
> >
> > Regards
> > Shi Hui
> >
> > On Thu, Oct 27, 2011 at 5:25 AM, Min Zhao <mzhao...@gmail.com> wrote:
> >>
> >> Hi Hui,
> >>
> >> Your change looks good to me in general. I agree with your approach to
> >> have one NystromAliasAnalyzer in IPA phase, most importantly having
> >> one alias tag map, which makes alias query for WNs in different PUs
> >> easy. Just some minor comments:
> >>
> >> 1) You have some places to access the data member (e.g.,
> >> _IPAAliasTageMap) directly. Can you please use the get/set function to
> >> access them?
> >
> > _IPAAliasTageMap is only used in its belonging class AliasAnalyzer.
> > And similarly _aliasTagMap is also directly used in AliasAnalyzer, so can
> we
> > keep the alignment of coding stype in AliasAnalyzer? Leaving all member
> > directly used in its own class.
> >
> >>
> >> 2) why we need to set Write_ALIAS_CGNODE_Map to be true in the new
> >> NystromAliasAnalyzer constructor? Do we need to dump the WN to
> >> CGNodeID map?
> >
> > You're right.Write_ALIAS_CGNODE_Map is already set true in
> > IPA_NystromAliasAnalyzer constructor.
> > So there is no need to update Write_ALIAS_CGNODE_Map again in
> > NystromAliasAnalyzer constructor.
> > I have fix this in new patch.
> >
> >>
> >> 3) your two functions IPANodeCG()(one is get function, the other is
> >> set function) are called same name, which is a little bit confusing.
> >
> > This also keep align with the exsting code.
> > static ConstraintGraph *globalCG() { return
> globalConstraintGraph;
> > }
> > static void globalCG(ConstraintGraph *cg) { globalConstraintGraph = cg;
> }
> >
> > static CG_ST_IDX adjustCGstIdx(IPA_NODE *ipaNode, CG_ST_IDX cg_st_idx);
> >
> > static ConstraintGraph *IPANodeCG() { return
> > currentIPANodeConstraintGraph; }
> >
> > static void IPANodeCG(ConstraintGraph* cg) {
> > currentIPANodeConstraintGraph = cg; }
> >
> >>
> >> 4) should the map _aliasTagMap deleted before the _memPool is deleted?
> >
> >
> > Yes, this seems unreasonable code that mem_pool is freed before WN_Map is
> > removed.
> > This doesn't cause problem, because in IPA_WN_MAP_Delete, it only free
> > memory when pool is Malloc_Mem_Pool.
> > Otherwise it will free the already freed memory.
> > Fix the order of WN_MAP delete.
> >
> >>
> >> Thanks,
> >>
> >> Min
> >>
> >> On Thu, Jul 28, 2011 at 12:43 AM, Hui Shi <kalin....@gmail.com> wrote:
> >> > Hi, All
> >> >
> >> > Can gatekeepers help review this patch?
> >> >
> >> > When apply option -OPT:alias=field_sensitive -IPA:preopt=on, compiler
> >> > will
> >> > assert in NystromAliasAnalyzer.
> >> > NystromAliasAnalyzer assumes it is in be phase and try to get the IPA
> >> > constraint graph summary from IR file.
> >> >
> >> > To enable nystrom alias analyzer works with pre_optimizer in IPA phase
> >> > need
> >> > create
> >> > NystromAliasAnalyze from global contraint graph and IPA Node's local
> >> > constraint graph.
> >> >
> >> > This patch is:
> >> > 1. Create NystromAliasAnalyzer for PU in IPA phase.
> >> > 2. Provide alias query interface for WN nodes or alias tags in
> different
> >> > PU.
> >> >
> >> > Key decision is only use one NystromAliasAnalyzer instance in IPA
> phase.
> >> > 1. Multiple PU's alias tag in one alias tag space.1
> >> > 1. Ease alias query, if same alias tag for WN node in different,
> they
> >> > are
> >> > aliased.
> >> > 2. Ease alias query cache. NystromAliasAnalyzer cache alias query
> >> > result,
> >> > key is
> >> > alias tag pair. If create one NystromAliasAnalyzer for each PU.
> >> > It's
> >> > complex to
> >> > cache qurey of alias tags from different PU.
> >> > 3. Save space for alias tag's points_to set .
> >> > In different PU, there is alias tags that their points_to set is
> >> > euqivalent.
> >> > For example, actual and formal paramters' points to set is same.
> >> > Only use one copy points to set can save memory.
> >> > 2. AliasAnalyzer::aliasAnalyzer() is a static method.
> >> > Current code assumes only one alias analyzer instance.
> >> >
> >> > Implementation:
> >> > Add globals:
> >> > 1. Alias_Analyzer_in_IPA
> >> > set true in IPA_Preoptimize when Alias_Nystrom_Analyzer is on.
> >> > 2. Current_IPANode_File_PU_Idx
> >> > current IPA node's file PU index in IPA_Preoptimize.
> >> >
> >> > In aliasAnalyzer
> >> > Add following fields
> >> > 1. IPAWNAliasTagMap _IPAAliasTageMap; map WN node to its alias tag.
> >> > Key is UINT64, (IPA Node file_pu_idx << 32) | wn_map_id
> >> > 2. bool _ipaMode; indicate if NystromAliasAnalyzer is created in IPA
> >> > phase
> >> > 3. UINT32 _curFilePUIdx; // current processing IPA node's file and
> PU
> >> > index
> >> >
> >> > Update AliasAnalyzer::Create_Alias_Analyzer(ALIAS_CONTEXT &ac, WN
> *tree)
> >> > if Alias_Analyzer_in_IPA, set _ipaMode true and set _curFilePUIdx.
> >> >
> >> > Update interface
> >> > void setAliasTag(WN *wn, AliasTag tag)
> >> > AliasTag getAliasTag(const WN *wn) const
> >> > to
> >> > void setAliasTag(WN *wn, AliasTag tag, UINT32 filePUIdx=UINT32_MAX)
> >> > AliasTag getAliasTag(const WN *wn, UINT32 filePUIdx=UINT32_MAX) const
> >> > In _ipaMode, compose filePUIdx with wn_map_id as key, find or set
> alias
> >> > tag
> >> > map.
> >> >
> >> > In NystromAliasAnalyzer
> >> > Add new constructor NystromAliasAnalyzer::NystromAliasAnalyzer
> >> > only set _ipaMode is true.
> >> > Not build constraint graph or create alias tag.
> >> >
> >> > Update NystromAliasAnalyzer::genAliasTag(ST *st, INT64 offset, INT64
> >> > size,
> >> > bool direct)
> >> > When getting St_Info for ST.
> >> > 1. If ipaMode and St is global, search St_Info in IPA global
> constraint
> >> > graph.
> >> > 2. If ipaMode and St is local st, search St_Info in local constriant
> >> > graph,
> >> > key is
> >> > File PU idx + ST idx
> >> > 3. Not in ipaMode, search in local constraint graph, key is ST idx.
> >> >
> >> > In ConstraintGraph
> >> > Add static ConstraintGraph *currentIPANodeConstraintGraph;
> >> > Pass current processing IPA node's constraint graph from
> >> > IPA_Preoptimize.
> >> > Used in AliasAnalyzer::Create_Alias_Analyzer, if in IPA phase, set
> >> > current
> >> > constraint graph with currentIPANodeConstraintGraph.
> >> >
> >> > Add bool _uniqueMapped; set true when current constraint graph is
> >> > processed
> >> > by
> >> > IPA_NystromAliasAnalyzer::mapWNToUniqCallSiteCGNodeId.
> >> >
> >> > IPA_NystromAliasAnalyzer::mapWNToUniqCallSiteCGNodeId currently only
> >> > called
> >> > before
> >> > write IPA nystrom alias info into IR file.
> >> > It is also needed called before create alias tag in IPA_preoptimize.
> >> > Use this flag to avoid map twice.
> >> >
> >> > Regards
> >> > Shi Hui
> >> >
> >> >
> >> >
> >> >
> >> >
> >> >
> ------------------------------------------------------------------------------
> >> > Got Input? Slashdot Needs You.
> >> > Take our quick survey online. Come on, we don't ask for help often.
> >> > Plus, you'll get a chance to win $100 to spend on ThinkGeek.
> >> > http://p.sf.net/sfu/slashdot-survey
> >> > _______________________________________________
> >> > Open64-devel mailing list
> >> > Open64-devel@lists.sourceforge.net
> >> > https://lists.sourceforge.net/lists/listinfo/open64-devel
> >> >
> >> >
> >
> >
>
------------------------------------------------------------------------------
Get your Android app more play: Bring it to the BlackBerry PlayBook
in minutes. BlackBerry App World™ now supports Android™ Apps
for the BlackBerry® PlayBook™. Discover just how easy and simple
it is! http://p.sf.net/sfu/android-dev2dev
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
