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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Cool! I like the replay logic. It's a little alarming that we will replay arbitrary number of operations on an arbitrary number of phi inputs, but I'm *really* not worried here and the simplicity and clarity of the logic resulting from simple replay is great.

The remaining comments are just superficial stuff. If they make sense to you, feel free to go ahead and commit with those changes. If something isn't clear or needs more discussion, just lemme know.



================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:98-101
+  // Walk up in backward a list of instructions that
+  // have 'copy' semantics by 'stripping' the copies
+  // until a PHI node or an instruction of unknown kind
+  // is reached. Negation via xor is also handled.
----------------
Probably want to update this comment to explain that we're now recording the operations so we can replay them.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:135
+
+  auto Compute = [&](const APInt &Value) -> APInt {
+    APInt Result = Value;
----------------
Heh, for once, you can omit the return type! ;] Maybe add a brief comment explaining this lambda replays the operations on an APInt?

Or could name it something more explanatory like `ApplyOperations`


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:137
+    APInt Result = Value;
+    for (int I = Operations.size() - 1; I >= 0; I--) {
+      Instruction *Op = Operations[I];
----------------
You can use a range based for loop:

  for (Instruction *Op : llvm::reverse(Operations)) {


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:141-142
+      case Instruction::Xor: {
+        ConstantInt *CInt = dyn_cast<ConstantInt>(Op->getOperand(1));
+        assert(CInt && "Expect constant operand 1");
+        Result ^= CInt->getValue();
----------------
Just use `cast` here, and likely skip the variable.


================
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:186-191
+    bool IsUnlikely = (ExpectedPhiValue != Compute(CI->getValue()));
+
+    // Not an interesting case when IsUnlikely is false -- we can not infer
+    // anything useful when the operand value matches the expected phi
+    // output.
+    if (!IsUnlikely)
----------------
I would sink the comparison into the condition. The variable 'IsUnlikely' isn't as helpful as the comment here, and its only used for the if.


https://reviews.llvm.org/D33164