[PATCH] D132408: [SimplifyCFG] accumulate bonus insts cost

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:242-243
+  // of common destination.
+  DenseMap<BasicBlock *, unsigned> *NumBonusInsts;
+  DenseMap<BasicBlock *, unsigned> LocalNumBonusInsts;
 
----------------
yaxunl wrote:
> tra wrote:
> > yaxunl wrote:
> > > tra wrote:
> > > > Do we ever need to clear the map carried by `SimplifyCFGOpt`?
> > > > Unlike the maps instantiated as a local variable in other places, the lifetime of `SimplifyCFGOpt` is less certain. 
> > > > 
> > > > If we do need to clear them, should we be doing that to the map given to us by the user, or only to the local one?
> > > > 
> > > > Do we ever need to clear the map carried by `SimplifyCFGOpt`?
> > > > Unlike the maps instantiated as a local variable in other places, the lifetime of `SimplifyCFGOpt` is less certain. 
> > > > 
> > > > If we do need to clear them, should we be doing that to the map given to us by the user, or only to the local one?
> > > > 
> > > 
> > > SimplifyCFGOpt is used by higher-level passes which work on functions or loops. The pass which uses SimplifyCFGOpt repeatedly is responsible for creating NumBonusInsts and passing it to SimplifyCFGOpt. NumBonusInsts is valid for the life cycle of the higher-level pass. It is only updated when there are new BB's folded. Once folded, the folded BB's are gone and will not be double counted in the future. Therefore NumBonusInsts needs not be emptied.
> > > 
> > > The LocalNumBonusInsts is always empty when SimplifyCFGOpt. It is used for situations where SimplifyCFG is used to simplify CFG's which do not fold branches or is not concerned with accumulated bonus insts. It degenerates to the original behaviour, i.e. only consider bonus insts of the current BB.
> > > Once folded, the folded BB's are gone and will not be double counted in the future. Therefore NumBonusInsts needs not be emptied.
> > 
> > OK.
> > 
> > Further question -- is there a possibility that a BB we've recorded in the map gets deleted (e.g. after merging into another BB), and then another BB gets created, reusing the same pointer?  That would result in erroneously counting in the old instance's bonus instructions. 
> > 
> > It may not be likely to happen and the effect would be that SimplifyCfg would hit the bonus threshold sooner than it should have normally, so it's probably not fatal.
> > On the other hand, it may make compilation nondeterministic -- if/when we hit such a pointer reuse we'll end up generating different code and that's more of a problem.
> > 
> > 
> > 
> > 
> > 
> > > The LocalNumBonusInsts is always empty when SimplifyCFGOpt. 
> > 
> > Looks like something got lost during editing. I assume you meant "when SimplifyCFGOpt is constructed with user-supplied map".
> > 
> > 
> > 
> > 
> > 
> > Further question -- is there a possibility that a BB we've recorded in the map gets deleted (e.g. after merging into another BB), and then another BB gets created, reusing the same pointer?  That would result in erroneously counting in the old instance's bonus instructions. 
> > 
> > It may not be likely to happen and the effect would be that SimplifyCfg would hit the bonus threshold sooner than it should have normally, so it's probably not fatal.
> > On the other hand, it may make compilation nondeterministic -- if/when we hit such a pointer reuse we'll end up generating different code and that's more of a problem.
> > 
> I can use CallbackVH, which will automatically update the map when the BB is deleted or replaced.
> > 
> > Further question -- is there a possibility that a BB we've recorded in the map gets deleted (e.g. after merging into another BB), and then another BB gets created, reusing the same pointer?  That would result in erroneously counting in the old instance's bonus instructions. 
> > 
> > It may not be likely to happen and the effect would be that SimplifyCfg would hit the bonus threshold sooner than it should have normally, so it's probably not fatal.
> > On the other hand, it may make compilation nondeterministic -- if/when we hit such a pointer reuse we'll end up generating different code and that's more of a problem.
> > 
> I can use CallbackVH, which will automatically update the map when the BB is deleted or replaced.

Actually, I will use ValueMap, which uses CallbackVH to remove the BB from the map when it is deleted. It is simpler.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132408/new/

https://reviews.llvm.org/D132408