[PATCH] D48754: recognize more abs pattern

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:104-105
 
+  /// Return true if the two given values are complement
+  bool isKnownComplement(const Value *LHS, const Value *RHS);
+
----------------
Doesn't "complement" mean bitwise-not? I think this should be called "isKnownNegation" unless I'm mixing up the terminology.

Nit: add a period at end of comment.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4506
 
+/// Return true if LHS and RHS are complement, otherwise return false
+bool llvm::isKnownComplement(const Value *LHS, const Value *RHS) {
----------------
Don't repeat the comment from the header file.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4508-4509
+bool llvm::isKnownComplement(const Value *LHS, const Value *RHS) {
+  if (!LHS || !RHS)
+    return false;
+
----------------
Make that an assert? Other functions in this file just assume that the parameters are valid. Ideally, we'd change everything to references instead of pointers...


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4515
+
+  // X = sub (a, b), Y = sub (b, a)
+  Value *X = nullptr, *Y = nullptr;
----------------
The comments don't match the code. The parameters to this function should be named X and Y, and these local values should be named A and B?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4516
+  // X = sub (a, b), Y = sub (b, a)
+  Value *X = nullptr, *Y = nullptr;
+  if (match(LHS, m_Sub(m_Value(X), m_Value(Y))) &&
----------------
Don't initialize these to nullptr. They should not be read if they are uninitialized, so explicitly initializing them removes a chance to produce a warning for that situation.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4521
+
+  //TODO: add more pattern here
+  return false;
----------------
Remove the TODO or specify a pattern that you know should be added later.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:815-817
   if (match(Cmp.getOperand(1), m_ZeroInt()) &&
       Cmp.getPredicate() == ICmpInst::ICMP_SLT)
     return nullptr;
----------------
We should check the negated value in the select here. If it is not m_Neg(), then we should create a new sub (neg) in the canonical form.

If you would prefer to make that a separate patch, then please add a 'TODO' comment here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:826
   Value *FVal = Sel.getFalseValue();
+  assert(isKnownComplement(TVal, FVal) && "Unexpected result from matchSelectPattern");
   if (SPF == SelectPatternFlavor::SPF_NABS) {
----------------
Formatting: goes over 80-columns.


================
Comment at: llvm/test/Transforms/InstCombine/abs-1.ll:119-123
+define i32 @abs_canonical_6(i32 %a, i32 %b) {
+; CHECK-LABEL: @abs_canonical_6(
+; CHECK-NEXT:    [[TMP:%.*]] = sub i32 [[A:%.*]], [[B:%.*]] 
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP]], 0
+; CHECK-NEXT:    [[NEG:%.*]] = sub i32 [[B]], [[A]]
----------------
Please commit all new tests to trunk with the current output. Then rebase this patch so we see only the diffs.


https://reviews.llvm.org/D48754