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

David Li via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 26 21:25:07 PDT 2017


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


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:100
+  bool ValueInverted = false;
+  // Walk up in backward a list of instructions that
+  // have 'copy' semantics by 'stripping' the copies
----------------
chandlerc wrote:
> chandlerc wrote:
> > nit: Some vertical whitespace before this comment block would make it easier to read IMO.
> nit: This is a blonk comment line rather than a blank line?
Removed the comment marker


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:144-145
+
+  bool IsPhiValueOne = ((!ValueInverted && !ExpectedValue->isZero()) ||
+                        (ValueInverted && ExpectedValue->isZero()));
+
----------------
chandlerc wrote:
> chandlerc wrote:
> > davidxl wrote:
> > > 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 
> > I don't think a TODO is really enough. We shouldn't put completely wrong probabilities on something just because we don't handle non-i1 expects. I'm fine if you just want to check at the start of this that the type is i1 and return otherwise.
> I'm still not seeing any response to my follow-up.
I am not sure what your followup question is, but non-i1 case in the patch is handled correctly (but conservatively) -- basically no wrong probability will be emitted -- and it will early return as you expected (isUnlikely will be false).  I enhanced the comment.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:200-203
+    // Returns true of Phi's operand 'opnd' is coming from 'succ' th
+    // successor of BI:
+    auto IsOpndComingFromSuccessor = [&](int opnd, int succ) {
+      auto *OpndIncomingBB = PhiDef->getIncomingBlock(opnd);
----------------
chandlerc wrote:
> I think this API is overall a bit confusing because we call it with integer indices, but it captures a specific branch and phi instruction...
> 
> Maybe, rather than getting the incoming block inside the lambda, why not pull the computation of `OpndIncomingBB` out of the lambda entirely? It doesn't change between the calls below.
> 
> Similarly, I feel like it would be a bit more readable to pass in the successor basic block and call `BI->getSuccessor()` in the call as that avoids documenting the magical 1 and 0 indices.
I modified the function.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:318
       if (Fn && Fn->getIntrinsicID() == Intrinsic::expect) {
+        // Before erasing the phi, walk backward to find
+        // phi that define builin_expect's first arg, and
----------------
chandlerc wrote:
> Again, I think 'Before erasing the phi' is confusing here. This code doesn't erase the phi node. I think this should say 'Before erasing the llvm.expect call, ...' which can then make it easier to refer to the first argument in the rest of the sentence.
Ouch,  I meant llvm.expect.


https://reviews.llvm.org/D33164





More information about the llvm-commits mailing list