[PATCH] D16836: [CodeGenPrepare] Don't transform select instructions into branches when both of operands are cheap

[Sanjay Patel via llvm-commits]

spatel added a comment.

In http://reviews.llvm.org/D16836#355287, @flyingforyou wrote:

> And I think we can use `getSchedModel().isOutOfOrder()` for setting the flag `PredictableSelectIsExpensive`.


Thanks, Junmo. I didn't realize that was possible, but that solves a problem. We shouldn't be using bogus predicates which approximate the necessary condition...the correct predicate is already available!

  Index: lib/Target/ARM/ARMISelLowering.cpp
  ===================================================================
  --- lib/Target/ARM/ARMISelLowering.cpp	(revision 261182)
  +++ lib/Target/ARM/ARMISelLowering.cpp	(working copy)
  @@ -1042,7 +1042,7 @@
     setMinStackArgumentAlignment(4);
   
     // Prefer likely predicted branches to selects on out-of-order cores.
  -  PredictableSelectIsExpensive = Subtarget->isLikeA9();
  +  PredictableSelectIsExpensive = Subtarget->getSchedModel().isOutOfOrder();
  
     setMinFunctionAlignment(Subtarget->isThumb() ? 1 : 2);
   }

And I see now that PredictableSelectIsExpensive defaults to 'false', so that means targets must opt-in to this transform. That makes me feel better about doing this transform in general, but it doesn't solve the problem raised by this patch.

The transform is based on a heuristic, and this patch may limit when that heuristic fires, but Zia and Mitch have suggested the opposite: they'd like to do this transform more often for their targets.

If we're going to solve that tension here in CGP, we're going to need another target hook or make PredictableSelectIsExpensive more than a bool value. To solve this at the machine-level, you'd either need to customize the transform per target or add that same target hook to be used from a common codegen pass.

I'm not sure which is the better option. As Mitch pointed out, there's certainly demand to do the transform(s) here because it's easier. And now that I realize we have access to the same SchedModel info that we would use at the machine-level anyway, it seems more reasonable. However, as Hal and Mehdi have noted, breaking up a basic block may harm isel/scheduling.


http://reviews.llvm.org/D16836