[CodeGenPrepare.cpp] Feedback on changing logic when to convert select to branch

[Gerolf]

Summary

The current logic in OptimizeSelectInst() converts a
select to a branch when the select is expensive (determined by a (sub-)target
specific flag) or the branch is cheaper (by some heuristic). The proposed change
enables the heuristic check to kick in even when the target specific flag
suggest any select is cheap and therefore should not be converted. Eventually we want good
heuristics when to prefer a branch over a select. Changing the condition when to
enable them is the first step doing so. At least on cyclone treating all selects
as 'cheap' results in regressions. I'm still evaluating the peformance impact of this change,
and look for feedback on the general direction.

[jmolloy]

Hi Gerolf,

The concept looks fine to me, there's no real functionality change here. One inline comment though.

James

lib/CodeGen/CodeGenPrepare.cpp

3069

Don't you want "isFormingBranchFromSelectProfitable(SI)" here?

[hfinkel]

This change concerns me, there are certainly targets on which the branch really is always more expensive. If you need to make the TLI interface more expressive, then feel free to do so, but don't remove a target's ability to turn off the select to branch conversion.

[Gerolf]

@hfinkel

The proposed change would make this very easy if it turns out to be necessary. In this case for architectures where the branch is *always* more expensive make sure
a) PredictableSelectIsExpensive=false (default for all arch)
b) add a target specific call in isFormingBranchFromSelectProfitable(SI) that overrides any heuristic by saying something like “select is always cheaper than branch”.

For which targets do you suggest adding b)? The only possible candidates are !Cortex, !"LikeA9", Atom:

./lib/CodeGen/TargetLoweringBase.cpp: PredictableSelectIsExpensive = false; (default)
./lib/Target/AArch64/AArch64ISelLowering.cpp: if (Subtarget->isCortexA57()) PredictableSelectIsExpensive = true;
./lib/Target/ARM/ARMISelLowering.cpp: PredictableSelectIsExpensive = Subtarget->isLikeA9();
./lib/Target/R600/AMDGPUISelLowering.cpp: PredictableSelectIsExpensive = false; (looks redundant)
./lib/Target/X86/X86ISelLowering.cpp: PredictableSelectIsExpensive = !Subtarget->isAtom();

FWIW, I have not found a regression on x86 or ARM for the change.

[chandlerc]

Removing myself as reviewer, Hal has pointed out a specific concern with this patch.

lib/CodeGen/CodeGenPrepare.cpp

bool CodeGenPrepare::OptimizeSelectInst(SelectInst *SI) {
   TargetLowering::SelectSupportKind SelectKind;
   if (VectorCond)
     SelectKind = TargetLowering::VectorMaskSelect;
   else if (SI->getType()->isVectorTy())
     SelectKind = TargetLowering::ScalarCondVectorVal;
   else
     SelectKind = TargetLowering::ScalarValSelect;
 
-  // Do we have efficient codegen support for this kind of 'selects' ?
-  if (TLI->isSelectSupported(SelectKind)) {
-    // We have efficient codegen support for the select instruction.
-    // Check if it is profitable to keep this 'select'.
-    if (!TLI->isPredictableSelectExpensive() ||
-        !isFormingBranchFromSelectProfitable(SI))
-      return false;
-  }
+  // When the target architecture does not support select, or the select
+  // is expensive, or heuristics suggest branches are  more profitable then
+  // turn the select into a branch
+  // FIXME: isFormingBranchSelectProfitable() should be target specific also.
+  bool ConvertToBranch = !TLI->isSelectSupported(SelectKind) ||
+                         TLI->isPredictableSelectExpensive() ||
+                         isFormingBranchFromSelectProfitable;
+  if (!ConvertToBranch)
 
   ModifiedDT = true;
 
   // First, we split the block containing the select into 2 blocks.
   BasicBlock *StartBlock = SI->getParent();
   BasicBlock::iterator SplitPt = ++(BasicBlock::iterator(SI));
   BasicBlock *NextBlock = StartBlock->splitBasicBlock(SplitPt, "select.end");