[PATCH] D68408: [InstCombine] Negator - sink sinkable negations

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

@efriedma - do you want to continue reviewing?

I've just pointed out a few nits for now.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineInternal.h:998
+// As a default, let's assume that we want to be somewhat aggressive,
+// and attemt to traverse up to 32 layers in attempt to sink negation.
+static constexpr unsigned NegatorDefaultMaxDepth = 32;
----------------
typo: attempt


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:168
+  case Instruction::Trunc: {
+    // `trunc` is negatible if it's operand is negatible.
+    Value *NegOp = visit(I->getOperand(0), Depth + 1);
----------------
it's -> its


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:182
+  case Instruction::Add: {
+    // `add` is negatible if both of it's operands are negatible.
+    Value *NegOp0 = visit(I->getOperand(0), Depth + 1);
----------------
it's -> its


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:192
+  case Instruction::Mul: {
+    // `mul` is negatible one of it's operands is negatible.
+    Value *NegatedOp, *OtherOp;
----------------
negatible one -> negatible if one


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:192
+  case Instruction::Mul: {
+    // `mul` is negatible one of it's operands is negatible.
+    Value *NegatedOp, *OtherOp;
----------------
spatel wrote:
> negatible one -> negatible if one
it's -> its


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineNegator.cpp:243
+
+  // We must temporairly unset the 'current' insertion point and DebugLoc of the
+  // InstCombine's IRBuilder so that it won't interfere with the ones we have
----------------
typo: temporarily


================
Comment at: llvm/test/Transforms/InstCombine/sub-of-negatible.ll:159
 ; Subtraction can be negated if the first operand can be negated
-; x - (y - z) -> x - y + z -> x + (-y) + z
-define i8 @t9(i8 %x, i8 %y, i8 %z) {
+; x - (y - z) -> x - y + z -> x + (z - y)
+define i8 @t9(i8 %x, i8 %y) {
----------------
I didn't follow the diffs here - was one of these tests redundant? The code comment didn't match before, but it still doesn't?


================
Comment at: llvm/test/Transforms/InstCombine/sub-of-negatible.ll:271-272
+
+; Phi can be negated if all incoming values can be negated
+define i8 @t16(i1 %c, i8 %x) {
+; CHECK-LABEL: @t16(
----------------
Add these tests with baseline results as pre-commit?


================
Comment at: llvm/test/Transforms/InstCombine/sub-of-negatible.ll:351
+
+; truncation can be negated if it's operand can be negated
+define i8 @t20(i8 %x, i16 %y) {
----------------
it's -> its


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68408