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

[David Li via Phabricator via llvm-commits]

davidxl marked 9 inline comments as done.
davidxl added inline comments.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:113
+
+        assert(!dyn_cast<ConstantInt>(Opnd0) && "expression not canonicalized");
+        if (ConstantInt *CInt = dyn_cast<ConstantInt>(Opnd1)) {
----------------
chandlerc wrote:
> You can't assert that IR is in canonical form. You can only assert that it would pass the verifier.
> 
> But you don't need to *handle* non-canonical form, you can just fail to add the metadata.
Removed.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:115-117
+          V = Opnd0;
+          if (CInt->getZExtValue())
+            ValueInverted = !ValueInverted;
----------------
chandlerc wrote:
> If you just want to test for zero, you can do that directly without zext-ing the constant...
This is not for constant but the value flow.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:134-135
+
+  if (!PhiDef)
+    return;
+
----------------
chandlerc wrote:
> You could just use one variable above (`V` or `Phi`) and write the loop to essentially be:
> 
>   // Walk up through the operands until we find a PHI node.
>   while (!isa<PHINode>(V)) {
>     ...
>   }
> 
>   auto *PN = dyn_cast<PHINode>(V);
>   if (!PN)
>     // Nothing to do if we cannot find a PHI node.
>     return;
The loop is not greatly simplified and more readable.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:147-150
+    // CFG may not be simplified yet, walk up one level
+    BB = BB->getSinglePredecessor();
+    if (!BB)
+      return nullptr;
----------------
chandlerc wrote:
> If the CFG isn't simplified, we need to walk more than one. If it is, we don't need to walk any. Do you have cases where this was necessary?
The comment is stale. We need to handle diamond shaped control.  See also phi_tern.ll for an example (where cfg is not simplified).


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:279-282
+        // Before erasing the phi, walk backward to find
+        // phi that define builin_expect's first arg, and
+        // infer branch probability:
+        handlePhiDef(CI);
----------------
chandlerc wrote:
> This doesn't erase a PHI node though... Maybe this comment just needs updating?
It is referring to the removal after this call.


https://reviews.llvm.org/D33164