[PATCH] D48754: [InstCombine] canonicalize abs pattern

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:797-799
 /// There are 4 select variants for each of ABS/NABS (different compare
 /// constants, compare predicates, select operands). Canonicalize to 1 pattern.
 /// This makes CSE more likely.
----------------
This comment is not accurate now. It should mention the possibility of a compare that uses the negated op. I'm not sure how many variants we can match now, but you can include that number if you know the answer.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:819-820
+
+  // Need to consider about pattern like RHS = sub (A, B), so can not use RHS to
+  // match operand 0.
+  bool CmpUsesNegatedOp = match(Cmp.getOperand(0), m_Neg(m_Specific(TVal))) ||
----------------
Rephrase this as:
The compare may use the negated abs()/nabs() operand, or it may use negation in non-canonical form such as: sub A, B.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:821-822
+  // match operand 0.
+  bool CmpUsesNegatedOp = match(Cmp.getOperand(0), m_Neg(m_Specific(TVal))) ||
+                      match(Cmp.getOperand(0), m_Neg(m_Specific(FVal)));
 
----------------
Indentation is not as expected.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:838
+  if (!match(RHS, m_Neg(m_Specific(LHS)))) {
+    assert(match(RHS, m_Sub(m_Value(), m_Value())) &&
+           "RHS should be negated value");
----------------
This assert can be more specific? The sub must be with the specific operands of LHS reversed?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:840
+           "RHS should be negated value");
+    RHS = Builder.CreateSub(ConstantInt::getNullValue(LHS->getType()), LHS);
+    if (TVal == LHS) {
----------------
Use Builder.CreateNeg().


================
Comment at: llvm/test/Transforms/InstCombine/abs-1.ll:188-192
+  %tmp1 = sub i32 %a, %b
+  %cmp = icmp sgt i32 %tmp1, -1
+  %tmp2 = sub i32 %b, %a
+  %abs = select i1 %cmp, i32 %tmp1, i32 %tmp2
+  %add = add i32 %abs, %tmp2 ; increase use count for %tmp2.
----------------
This is the extra-uses scenario that I was worried about. We have more instructions than we started with. We should not canonicalize if we can not have equal or less-than instructions from the transform.


================
Comment at: llvm/test/Transforms/InstCombine/abs-1.ll:344-349
+; CHECK-NEXT:    [[TMP1:%.*]] = sub i32 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP1]], 0
+; CHECK-NEXT:    [[TMP2:%.*]] = sub i32 [[B]], [[A]]
+; CHECK-NEXT:    [[NEGTMP:%.*]] = sub i32 0, [[TMP1]]
+; CHECK-NEXT:    [[ABS:%.*]] = select i1 [[CMP]], i32 [[TMP1]], i32 [[NEGTMP]]
+; CHECK-NEXT:    [[ADD:%.*]] = add i32 [[TMP2]], [[ABS]]
----------------
Similar to above - we should not ever have more instructions than we started with.


https://reviews.llvm.org/D48754