[PATCH] D33164: [Profile[ Enhance expect lowering to handle correlated branches

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Overall looks good. Some minor comments inlined.



================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:112-119
+        if (ConstantInt *CInt = dyn_cast<ConstantInt>(Opnd0)) {
+          V = Opnd1;
+          if (CInt->getZExtValue())
+            ValueInverted = !ValueInverted;
+        } else if (ConstantInt *CInt = dyn_cast<ConstantInt>(Opnd1)) {
+          V = Opnd0;
+          if (CInt->getZExtValue())
----------------
BinaryOp should already be canonicalized here, so ConstantInt will be in Opnd1.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:142
+
+  // Get the first docminating conditional branch of the operand
+  // i's incoming block.
----------------
docminating --> dominating


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:159
+
+  // Now wolk through all Phi operands:
+  for (unsigned i = 0, e = PhiDef->getNumIncomingValues(); i != e; ++i) {
----------------
wolk -> walk


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:162-164
+    if (!isa<ConstantInt>(PhiOpnd))
+      continue;
+    bool IsPhiOpndOne = !dyn_cast<ConstantInt>(PhiOpnd)->isZero();
----------------
isa<ConstantInt> (PhiOpnd) can be replaced by 
ConstantInt *CI; 
if (CI = dyn_cast<ConstantInt>(PhiOpnd)) 


================
Comment at: test/Transforms/LowerExpectIntrinsic/phi_merge.ll:25
+  %tmp9 = icmp slt i32 %tmp7, %tmp8
+; CHECK: !prof [[WEIGHT:![0-9]+]]
+  br i1 %tmp9, label %bb10, label %bb18
----------------
Maybe add the whole br statement? Otherwise we may check a !prof annotated to a different branch other than we expect. 


https://reviews.llvm.org/D33164