[PATCH] D15987: [PPC] Handle weighted comparisons when inserting selects.

[Kyle Butt via llvm-commits]

iteratee added a comment.

I've cleaned up the unneeded attributes from the testcase, and will upload them shortly.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:747
@@ -746,3 +746,3 @@
   unsigned OpCode = Is64Bit ? PPC::ISEL8 : PPC::ISEL;
-  unsigned SelectPred = Cond[0].getImm();
+  auto SelectPred = static_cast<PPC::Predicate>(Cond[0].getImm());
 
----------------
hfinkel wrote:
> On architectures where isel is not always cheaper than the branch sequence, we need to be careful here. In fact, Kit is already working on code to expand isels late into branches to help the branch predictor do the right thing on P7/P8/etc. Enabling this conversion is good for some cores where isel really is cheap (on the PPC A2 for example), but I highly suspect that for P7/P8/etc. we should just reject the transformation entirely as to not lose the hinting information.
> 
> Thus, please either add code (or a FIXME) in canInsertSelect that we should not be converting hinted branches into isels on the P7/P8/etc.
> 
> I think the cleanest way to do all this might be to:
> 
>  1. Implement a function that gives the non-hinted predicate from a hinted predicate, say, getNonHintedPredicate. Then, here, we just need:
> 
>   switch(getNonHintedPredicate(SelectPred)) {
> 
> and in canInsertSelect, we can have:
> 
>   unsigned Directive =
>     DAG->MF.getSubtarget<PPCSubtarget>().getDarwinDirective();
>   unsigned SelectPred = Cond[0].getImm();
>   // Don't convert hinted branches into isel on the P7, and similar because losing the hinting information is likely worse than any benefit the isel brings.
>   if (getNonHintedPredicate(SelectPred) != SelectPred && (Directive == PPC::DIR_PWR7 || Directive == PPC::DIR_PWR8))
>     return false;
> 
I agree that we should only perform this when it's a performance win, but I don't like the rest of the approach.
I think the switch should cover all cases and use an enum, as this bug would have been caught in that case.
For canInsertSelect, I think isHintedPredicate(SelectPred) is much cleaner as well.

I'm not sure that using a list of processor directives is right either. Can you suggest a processor feature that we can add to make this happen, like I did with HasFusion? This reduces the maintenance burden of adding a new subtarget.


http://reviews.llvm.org/D15987