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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:2051
   int BudgetRemaining =
     PHINodeFoldingThreshold * TargetTransformInfo::TCC_Basic;
   for (PHINode &PN : EndBB->phis()) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86346