[PPC] Handle weighted comparisons when inserting selects.

[iteratee]

Summary

Only non-weighted predicates were handled in PPCInstrInfo::insertSelect. Handle the weighted predicates as well.

This latent bug was triggered by r255398, because it added use of the branch-weighted predicates.

While here, switch over an enum instead of an int to get the compiler to enforce totality in the future.

[echristo]

I think you can trim down the lists of attributes in the testcase, also needs a more descriptive description of what the bug is and how this is fixing it - mostly, can you elaborate more? I know what it's fixing, but for the commit it's better to be more descriptive.

Thanks!

-eric

[hfinkel]

lib/Target/PowerPC/PPCInstrInfo.cpp

747

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;

[iteratee]

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

lib/Target/PowerPC/PPCInstrInfo.cpp

747

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.

[hfinkel]

lib/Target/PowerPC/PPCInstrInfo.cpp

747

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.

Good point. I'm fine with HasSlowISEL (to go along with our HasISEL). Thanks!

[iteratee]

I have patches for the requested fixes out. Are there any other issues on this patch?

[echristo]

Not from me, the testcase looks fine here with the one nit I had.

Go ahead and wait for Hal first please :)

-eric

test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll

34

Not used :)

[iteratee]

remove unused #0 attributes.

[hfinkel]

LGTM too, thanks!

lib/Target/PowerPC/PPCInstrInfo.cpp

void PPCInstrInfo::insertSelect(MachineBasicBlock &MBB,
   bool Is64Bit = PPC::G8RCRegClass.hasSubClassEq(RC) ||
                  PPC::G8RC_NOX0RegClass.hasSubClassEq(RC);
   assert((Is64Bit ||
           PPC::GPRCRegClass.hasSubClassEq(RC) ||
           PPC::GPRC_NOR0RegClass.hasSubClassEq(RC)) &&
          "isel is for regular integer GPRs only");
 
   unsigned OpCode = Is64Bit ? PPC::ISEL8 : PPC::ISEL;
-  unsigned SelectPred = Cond[0].getImm();
+  auto SelectPred = static_cast<PPC::Predicate>(Cond[0].getImm());
 
   unsigned SubIdx;
   bool SwapOps;
   switch (SelectPred) {
-  default: llvm_unreachable("invalid predicate for isel");
-  case PPC::PRED_EQ: SubIdx = PPC::sub_eq; SwapOps = false; break;
-  case PPC::PRED_NE: SubIdx = PPC::sub_eq; SwapOps = true; break;
-  case PPC::PRED_LT: SubIdx = PPC::sub_lt; SwapOps = false; break;
-  case PPC::PRED_GE: SubIdx = PPC::sub_lt; SwapOps = true; break;
-  case PPC::PRED_GT: SubIdx = PPC::sub_gt; SwapOps = false; break;
-  case PPC::PRED_LE: SubIdx = PPC::sub_gt; SwapOps = true; break;
-  case PPC::PRED_UN: SubIdx = PPC::sub_un; SwapOps = false; break;
-  case PPC::PRED_NU: SubIdx = PPC::sub_un; SwapOps = true; break;
+  case PPC::PRED_EQ:
+  case PPC::PRED_EQ_MINUS:
+  case PPC::PRED_EQ_PLUS:
+      SubIdx = PPC::sub_eq; SwapOps = false; break;
+  case PPC::PRED_NE:
+  case PPC::PRED_NE_MINUS:
+  case PPC::PRED_NE_PLUS:
+      SubIdx = PPC::sub_eq; SwapOps = true; break;
+  case PPC::PRED_LT:
+  case PPC::PRED_LT_MINUS:
+  case PPC::PRED_LT_PLUS:
+      SubIdx = PPC::sub_lt; SwapOps = false; break;
+  case PPC::PRED_GE:
+  case PPC::PRED_GE_MINUS:
+  case PPC::PRED_GE_PLUS:
+      SubIdx = PPC::sub_lt; SwapOps = true; break;
+  case PPC::PRED_GT:
+  case PPC::PRED_GT_MINUS:
+  case PPC::PRED_GT_PLUS:
+      SubIdx = PPC::sub_gt; SwapOps = false; break;
+  case PPC::PRED_LE:
+  case PPC::PRED_LE_MINUS:
+  case PPC::PRED_LE_PLUS:
+      SubIdx = PPC::sub_gt; SwapOps = true; break;
+  case PPC::PRED_UN:
+  case PPC::PRED_UN_MINUS:
+  case PPC::PRED_UN_PLUS:
+      SubIdx = PPC::sub_un; SwapOps = false; break;
+  case PPC::PRED_NU:
+  case PPC::PRED_NU_MINUS:
+  case PPC::PRED_NU_PLUS:
+      SubIdx = PPC::sub_un; SwapOps = true; break;
   case PPC::PRED_BIT_SET:   SubIdx = 0; SwapOps = false; break;
   case PPC::PRED_BIT_UNSET: SubIdx = 0; SwapOps = true; break;
   }
 
   unsigned FirstReg =  SwapOps ? FalseReg : TrueReg,
            SecondReg = SwapOps ? TrueReg  : FalseReg;
 
   // The first input register of isel cannot be r0. If it is a member

test/CodeGen/PowerPC/2016-01-07-BranchWeightCrash.ll

+; RUN: llc <%s | FileCheck %s
+target datalayout = "e-m:e-i64:64-n32:64"
+target triple = "powerpc64le-unknown-linux-gnu"
+
+%struct.buffer_t = type { i64, i8*, [4 x i32], [4 x i32], [4 x i32], i32, i8, i8, [2 x i8] }
+
+declare i32 @__f1(i8*, %struct.buffer_t* noalias)
+
+; CHECK-LABEL: f1:
+define i32 @f1(i8* %__user_context, %struct.buffer_t* noalias %f1.buffer) {
+entry:
+  br i1 undef, label %"assert succeeded", label %"assert failed", !prof !1
+
+"assert failed":                                  ; preds = %entry
+  br label %destructor_block
+
+"assert succeeded":                               ; preds = %entry
+  %__f1_result = call i32 @__f1(i8* %__user_context, %struct.buffer_t* %f1.buffer) #5
+  %0 = icmp eq i32 %__f1_result, 0
+  br i1 %0, label %"assert succeeded11", label %"assert failed10", !prof !1
+
+destructor_block:                                 ; preds = %"assert succeeded11", %"assert failed10", %"assert failed"
+  %1 = phi i32 [ undef, %"assert failed" ], [ %__f1_result, %"assert failed10" ], [ 0, %"assert succeeded11" ]
+  ret i32 %1
+
+"assert failed10":                                ; preds = %"assert succeeded"
+  br label %destructor_block
+
+"assert succeeded11":                             ; preds = %"assert succeeded"
+  br label %destructor_block
+}
+
+attributes #5 = { nounwind }
+
+!1 = !{!"branch_weights", i32 1073741824, i32 0}