[PATCH] D86346: [SimplifyCFG] Accumulate cost against budget

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2051
   int BudgetRemaining =
     PHINodeFoldingThreshold * TargetTransformInfo::TCC_Basic;
   for (PHINode &PN : EndBB->phis()) {
----------------
lebedev.ri wrote:
> samparker wrote:
> > lebedev.ri wrote:
> > > Please add a new `cl::opt` for that.
> > > I believe, the threshold should be the branch misprediction cost,
> > > i.e. if it takes less cost (latency) to execute the branch
> > > then to wrongly predict that it won't be executed,
> > > but then rewind and execute it anyways, we should just execute it.
> > > 
> > > So i'd go with `20`.
> > Most of the cost modelling framework actually tries to assume that branches are always predicated though, and I don't understand why making arbitrary decisions about machine details would be a good idea here. Plus, it's also completely irrelevant to code size...  It would seem to be better to query the latency / size of the branch, but I highly doubt that would produce results that anyone would want...
> I'll rephrase: given that the cost modelling logic changed,
> the remaining value of `PHINodeFoldingThreshold` is completely wrong.
> 
Okay... so I guess we need to start at: What is the purpose of this code? I think it's trying to convert any number of phis and speculate, at most, a single instruction (and maybe try to figure out something with constexprs). If PHINodeFoldingThreshold is supposed to represent the cost of a cmp + sel, this doesn't feel completely broken to me but the threshold could just be PHINodeFoldingThreshold * NumPHIs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86346