[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:
> > > 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?
> I have no idea, it's you who is changing/fixing this,
> you should know how it works, so you tell me :)
> 
> I'm just saying that since the entire cost-modelling logic
> changed from "let's just ensure that every single instruction
> is below some threshold, and ignore total cost"
> to "let's count the total cost", it is pretty obvious
> that using the old threshold as-is makes no sense.
Yeah, so I think multiplying by the number of phis should bring us back inline with what we were doing before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86346