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

[David Li via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:144-145
+
+  bool IsPhiValueOne = ((!ValueInverted && !ExpectedValue->isZero()) ||
+                        (ValueInverted && ExpectedValue->isZero()));
+
----------------
chandlerc wrote:
> How do you want this to handle non-i1 expect calls?
As other cases, arbitrary value builtin_expect is not yet properly handled.   One possible use of it is to annotate switch statement, but it is pretty uncommon.

I added a TODO there 


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:181-186
+    // Figuring out which outgoing edge from BR that leads
+    // to the PhiDef's parent block. If the CFG is triangular
+    // shaped and the the operand's incoming block is BR's parent
+    // block itself, Phi's parent BB is the direct successor
+    // of BR's parent; Otherwise check the incoming block of the
+    // operand to see it if is one BR's successors:
----------------
chandlerc wrote:
> Reading this loop, I felt like this could end up setting the same metadata on a branch multiple times...
> 
> But reading this I feel like this isn't actually the case. But I'm also not exactly sure how this is working. I mean, I understand at a high level the cases you're trying to handle (I've read the test cases), but it really isn't clear how *this* code is effectively doing that.
> 
> Maybe an ascii-art diagram of the two cases you're trying to handle here would help? Maybe specifically talking about why we're looping over all of the phi's operands? (I had asked for the latter already...)
I used a lambda function and I think it is clearer now.


https://reviews.llvm.org/D33164