[PATCH] D108837: [SimplifyCFG] Ignore free instructions when computing cost for folding branch to common dest

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

We handled any controversy in the earlier patches, so this is now the minimal patch as described. LGTM



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:3262-3263
+        TTI->getUserCost(&I, CostKind) != TargetTransformInfo::TCC_Free) {
+      // Account for the cost of duplicating this instruction into each
+      // predecessor. Ignore free instructions.
+      NumBonusInsts += PredCount;
----------------
Nit: I'd put the comment before the 'if' line that it explains. 


================
Comment at: llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-logical.ll:142
   %cmp14 = fcmp olt double %conv13, 0.000000e+00
   br i1 %cmp14, label %if.then, label %lor.lhs.false16
 
----------------
lebedev.ri wrote:
> aeubanks wrote:
> > lebedev.ri wrote:
> > > spatel wrote:
> > > > aeubanks wrote:
> > > > > lebedev.ri wrote:
> > > > > > aeubanks wrote:
> > > > > > > lebedev.ri wrote:
> > > > > > > > aeubanks wrote:
> > > > > > > > > Now this branch is getting folded into the next basic block. Then at the end of -O2 when every `fpext` is eliminated, the final simplifycfg will fold every branch (since each block only consists of at most one extra instruction besides the cmp and branch), except for this block which is now slightly bigger.
> > > > > > > > > Any ideas on how to fix this?
> > > > > > > > I do not understand why this test is being affected at all, there are no zero-cost instructions here?
> > > > > > > seems like we consider `%vecext17 = extractelement <4 x float> %t, i32 0` to be free
> > > > > > > 
> > > > > > > https://github.com/llvm/llvm-project/blob/063af63b9664151b3a9206feefa9a6a36a471e80/llvm/lib/Target/X86/X86TargetTransformInfo.cpp#L3433
> > > > > > > 
> > > > > > > I tried looking at the history, this special case seems very old
> > > > > > Ah, right, that makes sense.
> > > > > > Didn't look, but not sure there is a nice fix here.
> > > > > @spatel any thoughts on this?
> > > > It's unfortunately a regression, but as the related tests show, we're not getting ideal results (2 vector compares) on most examples.
> > > > 
> > > > The cost model is telling the truth from its limited perspective - the extract from elt 0 is free, but the rest are not (they require shuffles). 
> > > > 
> > > > We need to be able to view these as sequences rather than as individual instructions or basic blocks either here or in SLP to improve things.
> > > > 
> > > > A quick hack solution might be to adjust the bonus budget in the presence of vector ops. Ie, if code has vectors, we try harder to speculate instructions because we assume that the cost of branching is likely greater than it appears, and we recognize that creating larger basic blocks has positive impact on SLP.
> > > Now that simplifycfg does not speculate known not-taken branches,
> > > my next step was to introduce a multiplier to be appled to speculation budgets
> > > for known-taken branches.
> > > 
> > > So i think the vector hack makes sense.
> > what's the right way to check if an instruction is a vector op?
> I guess you want to add `Instruction::isVectorOp()`.
> Perhaps we just want to check that any of: produced type, argument types
> is a vector type?
Checking the types seems like it would do. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108837