[PATCH] D19674: [SimplifyCFG] propagate branch metadata when creating select

[Sanjay Patel via llvm-commits]

spatel created this revision.
spatel added reviewers: davidxl, hfinkel, bkramer.
spatel added a subscriber: llvm-commits.
Herald added a subscriber: mcrosier.

Hopefully, the change itself is straightforward - plug another hole where we drop branch weight metadata while creating a select.

I'm curious about the lack of use of BranchProbability in SimplifyCFG though. Is there a reason we don't use that here (and therefore need duplicate functionality like "FitWeights()")?

http://reviews.llvm.org/D19674

Files:
  lib/Transforms/Utils/SimplifyCFG.cpp
  test/Transforms/SimplifyCFG/2008-07-13-InfLoopMiscompile.ll

Index: test/Transforms/SimplifyCFG/2008-07-13-InfLoopMiscompile.ll
===================================================================
--- test/Transforms/SimplifyCFG/2008-07-13-InfLoopMiscompile.ll
+++ test/Transforms/SimplifyCFG/2008-07-13-InfLoopMiscompile.ll
@@ -17,8 +17,8 @@
 ; CHECK:       mooseblock:
 ; CHECK-NEXT:    [[CMPB:%.*]] = icmp eq i1 [[CMPA]], false
 ; CHECK-NEXT:    [[BRMERGE:%.*]] = or i1 [[CMPB]], [[CMPA]]
-; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPB]], i32 0, i32 2
-; CHECK-NEXT:    br i1 [[BRMERGE]], label %func_1.exit, label %infloop, !prof !1
+; CHECK-NEXT:    [[DOTMUX:%.*]] = select i1 [[CMPB]], i32 0, i32 2, !prof !1
+; CHECK-NEXT:    br i1 [[BRMERGE]], label %func_1.exit, label %infloop, !prof !2
 ; CHECK:       func_1.exit:
 ; CHECK-NEXT:    [[OUTVAL:%.*]] = phi i32 [ 1, %entry ], [ [[DOTMUX]], %mooseblock ]
 ; CHECK-NEXT:    [[POUT:%.*]] = tail call i32 (i8*, ...) @printf
@@ -59,5 +59,6 @@
 !3 = !{!"branch_weights", i32 7, i32 8}
 
 ; CHECK: !0 = !{!"branch_weights", i32 1, i32 2}
-; CHECK: !1 = !{!"branch_weights", i32 73, i32 32}
+; CHECK: !1 = !{!"branch_weights", i32 3, i32 4}
+; CHECK: !2 = !{!"branch_weights", i32 73, i32 32}
 
Index: lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- lib/Transforms/Utils/SimplifyCFG.cpp
+++ lib/Transforms/Utils/SimplifyCFG.cpp
@@ -2835,7 +2835,31 @@
   PBI->setSuccessor(0, CommonDest);
   PBI->setSuccessor(1, OtherDest);
 
-  // Update branch weight for PBI.
+  // OtherDest may have phi nodes.  If so, add an entry from PBI's
+  // block that are identical to the entries for BI's block.
+  AddPredecessorToBlock(OtherDest, PBI->getParent(), BB);
+
+  // We know that the CommonDest already had an edge from PBI to
+  // it.  If it has PHIs though, the PHIs may have different
+  // entries for BB and PBI's BB.  If so, insert a select to make
+  // them agree.
+  PHINode *PN;
+  for (BasicBlock::iterator II = CommonDest->begin();
+       (PN = dyn_cast<PHINode>(II)); ++II) {
+    Value *BIV = PN->getIncomingValueForBlock(BB);
+    unsigned PBBIdx = PN->getBasicBlockIndex(PBI->getParent());
+    Value *PBIV = PN->getIncomingValue(PBBIdx);
+    if (BIV != PBIV) {
+      // Insert a select in PBI to pick the right value.
+      // Transfer any original profile metadata from the branch to the select.
+      Value *NV = Builder.CreateSelect(PBICond, PBIV, BIV,
+                                       PBIV->getName() + ".mux", PBI);
+      PN->setIncomingValue(PBBIdx, NV);
+    }
+  }
+
+  // Update branch weight for PBI. We do this after the select creation above
+  // because we want to preserve the original branch weights for the select.
   uint64_t PredTrueWeight, PredFalseWeight, SuccTrueWeight, SuccFalseWeight;
   bool PredHasWeights =
       PBI->extractProfMetadata(PredTrueWeight, PredFalseWeight);
@@ -2860,28 +2884,6 @@
                          .createBranchWeights(NewWeights[0], NewWeights[1]));
   }
 
-  // OtherDest may have phi nodes.  If so, add an entry from PBI's
-  // block that are identical to the entries for BI's block.
-  AddPredecessorToBlock(OtherDest, PBI->getParent(), BB);
-
-  // We know that the CommonDest already had an edge from PBI to
-  // it.  If it has PHIs though, the PHIs may have different
-  // entries for BB and PBI's BB.  If so, insert a select to make
-  // them agree.
-  PHINode *PN;
-  for (BasicBlock::iterator II = CommonDest->begin();
-       (PN = dyn_cast<PHINode>(II)); ++II) {
-    Value *BIV = PN->getIncomingValueForBlock(BB);
-    unsigned PBBIdx = PN->getBasicBlockIndex(PBI->getParent());
-    Value *PBIV = PN->getIncomingValue(PBBIdx);
-    if (BIV != PBIV) {
-      // Insert a select in PBI to pick the right value.
-      Value *NV = cast<SelectInst>
-        (Builder.CreateSelect(PBICond, PBIV, BIV, PBIV->getName() + ".mux"));
-      PN->setIncomingValue(PBBIdx, NV);
-    }
-  }
-
   DEBUG(dbgs() << "INTO: " << *PBI->getParent());
   DEBUG(dbgs() << *PBI->getParent()->getParent());
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D19674.55444.patch
Type: text/x-patch
Size: 4031 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160428/abe4ca33/attachment.bin>