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

[David Li via Phabricator via llvm-commits]

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


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:136-139
+      if (CInt->isMinusOne())
+         ValueInverted = !ValueInverted;
+      else
+        break;
----------------
chandlerc wrote:
> This looks like it is wrong when the xor operand is neither zero nor -1 (possibly due to being on a zero-extended value). We move `V` to the operand which might be the phi, and then break out.
> 
> As I had suggested, I find this easier to read when structured using early return and it also avoids the bug:
> 
>   if (!CInt->isZero()) {
>     if (!CInt->isMinusOne())
>       return;
> 
>     ValueInverted = !ValueInverted;
>   }
> 
> That said, I would also personally only assign V to the operand after this block just to avoid confusion.
I made some flow changes to match previous patterns. It is more readable now.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:166
+
+  // Now walk through all Phi operands:
+  for (unsigned i = 0, e = PhiDef->getNumIncomingValues(); i != e; ++i) {
----------------
chandlerc wrote:
> An old comment that didn't get addressed but is on a different line now:
> """
> This comment isn't really helping me. From the code, I know you're walking the PHI operands, but I don't know why or what you're planning to do with them.
> """
Added more comments.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:175-177
+    // TODO: properly handlle non-i1 case. For instance, when the expected
+    // phi value is 10, and the phi operand value is 5, isUnlikely should
+    // be set to true.
----------------
chandlerc wrote:
> We typically use 'FIXME'. I'd put this comment up on the computing of `IsPhiValueNonZero` as that's when this became confusing for me.
> 
> I also don't think the `IsUnlikely` variable is useful here. I would just sink it into the `if`.
> 
> The comment `Not an interesting case:` doesn't really tell the reader much. I think it would be more helpful to explain why only matched operands and values are meaningful to turn into metadata.
Added more explanation. 


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:199
+       auto *Succ = BR->getSuccessor(succ);
+       if (OpndIncomingBB == Succ)
+         return true;
----------------
chandlerc wrote:
> Maybe comment:
> 
>   // If this successor is the incoming block for this Phi operand, then
>   // this successor does lead to the Phi.
> 
> However, I feel like this needs to check that `OpndIncomingBB->getUniquePredecessor()` returns non-null. Otherwise this might not be an actual diamond.
I added the comment. There is no need to check unique predecessor here though.  The existance of other predecessors does not invalidate the fact that any incoming edge to OpndIncomingBB is unlikely.


https://reviews.llvm.org/D33164