[PATCH] D12341: add llvm.unpredictable intrinsic and lower it to metadata

[Krzysztof Parzyszek via llvm-commits]

kparzysz added a comment.

Minor nitpicking (not always directly related to the code you've changed).  The change itself LGTM.


================
Comment at: lib/Transforms/Scalar/LowerPredictionIntrinsic.cpp:71
@@ -69,3 +70,3 @@
 
   SI.setCondition(ArgValue);
   return true;
----------------
See comment at 136.

================
Comment at: lib/Transforms/Scalar/LowerPredictionIntrinsic.cpp:82
@@ +81,3 @@
+  // branch 0, in other case more likely is branch 1.
+  if (ExpectedValue.isOne())
+    Node = MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight);
----------------
The expected value doesn't have to be 1 in the non-optimized case.  I'm not sure if optimized code will always have 1, but a check for non-zero would be more general.

================
Comment at: lib/Transforms/Scalar/LowerPredictionIntrinsic.cpp:114
@@ -92,3 +113,3 @@
   } else {
     if (CmpI->getPredicate() != CmpInst::ICMP_NE)
       return false;
----------------
Are we just hoping that the comparison will be against 0?  :)

================
Comment at: lib/Transforms/Scalar/LowerPredictionIntrinsic.cpp:136
@@ -109,14 +135,3 @@
 
-  MDBuilder MDB(CI->getContext());
-  MDNode *Node;
-
-  // If expect value is equal to 1 it means that we are more likely to take
-  // branch 0, in other case more likely is branch 1.
-  if (ExpectedValue->isOne())
-    Node = MDB.createBranchWeights(LikelyBranchWeight, UnlikelyBranchWeight);
-  else
-    Node = MDB.createBranchWeights(UnlikelyBranchWeight, LikelyBranchWeight);
-
-  BI.setMetadata(LLVMContext::MD_prof, Node);
-
+  Value *ArgValue = CI->getArgOperand(0);
   if (CmpI)
----------------
This part (136-140) is not really necessary here.  The pass will remove all prediction intrinsics later on anyways, including replacing appropriate values.  Handling that in one place would be more logical.


http://reviews.llvm.org/D12341