[PATCH] D48754: recognize more abs pattern

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D48754#1158271, @shchenz wrote:

> @spatel  Hi Sanjay, I have made changes accordingly. Could you help to have another review. Thanks.


Thanks for the changes. This looks mostly right, but I want to make sure we're testing everything properly. This is really several independent steps in 1 patch, so I'd like to reduce the risk of bugs by breaking it up.

1. Please separate the creation of isKnownNegation() into its own patch and use it from InstSimplify -> SimplifyAddInst(). I have added tests at https://reviews.llvm.org/rL336822, so that should be a very small patch. Improving InstSimplify helps several other passes, so that's a good patch independent of abs().
2. The changes in matchSelectPattern can be separated and tested using patterns that are optimized by InstCombiner::foldSPFofSPF(). We'll need to add some tests for that part. Let me know if it is not clear.
3. The canonicalization changes will be the last step and should be covered by the tests shown in this patch, but I think we're missing some cases where the code may have bugs.



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4640
     // Set LHS and RHS so that RHS is the negated operand of the select
+    bool cmpEqNegated = match(CmpLHS, m_Neg(m_Specific(TrueVal))) ||
+                        match(CmpLHS, m_Neg(m_Specific(FalseVal)));
----------------
cmpEqNegated -> CmpUsesNegatedOp ?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4672-4673
+
+    // (-X >s 0) ? -X : X and (-X >s -1) ? -X : X --> ABS(-X)
+    // (-X >s 0) ? X : -X and (-X >s -1) ? X : -X --> NABS(-X)
+    if (cmpEqNegated && Pred == ICmpInst::ICMP_SGT &&
----------------
Would it be better to return {N}ABS(X) here rather than {N}ABS(-X)? 

Why is it useful to return the negated operand as the input rather than the original operand X?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:840
+           "RHS should be negated value");
+    if (RHS == TVal) {
+      auto *TInst = dyn_cast<BinaryOperator>(TVal);
----------------
What happens if the subtract has other uses? We should have tests for that possibility.


https://reviews.llvm.org/D48754