[PATCH] D138542: [InstSimplify] Use dominate condtion to simplify instructions

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:744-745
 
+static Value *simplifyByDomEq(unsigned Opcode, Value *Op0, Value *Op1,
+                              const SimplifyQuery &Q) {
+  Optional<bool> Imp =
----------------
Put a descriptive comment on this function like:
   /// Test if there is a dominating equivalence condition for the
   /// two operands. If there is, try to reduce the binary operation
   /// between the two operands. 
   /// Example: Op0 - Op1 --> 0 when Op0 == Op1


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:762
+    case Instruction::And:
+    case Instruction::Or:
+      return Op1;
----------------
bcl5980 wrote:
> I'm a little worry about the `and`/`or` part. Recursive call to isImpliedByDomCondition looks a little heavy.
> I return Op1 here is because Op1 can be constant value but not sure it has any side effect or not.
Did you run any compile-time-tracker experiments?
We could leave those out of the initial patch, add a TODO, and then put them back in a follow-up patch to reduce risk.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:902-903
 
+  // if (X == Y)
+  //     X - Y -> 0
+  if (Value *V = simplifyByDomEq(Instruction::Sub, Op0, Op1, Q))
----------------
If there's a good block comment for the new function, comments like this at the callers are probably not needed.


================
Comment at: llvm/test/Transforms/InstSimplify/domcondition.ll:20
+  %xor = xor i32 %x, %y
+  %mul = mul i32 %xor, %x
+  br label %cond.end
----------------
The mul in these tests could be removed?

It would be better to still have at least one negative test. We want to make sure the implication is not inverted somehow.

Go ahead and pre-commit the baseline tests, so we just show diffs in this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138542/new/

https://reviews.llvm.org/D138542