[PATCH] D48754: [InstCombine] canonicalize abs pattern

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 4 inline comments as done.
shchenz 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.
----------------
spatel wrote:
> 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.
Maybe we want to add new pattern in isKnownNegation(), to avoid change the comments every time we change isKnownNegetion(), I don't use a accurate number. Hope this is ok.


================
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");
----------------
spatel wrote:
> This assert can be more specific? The sub must be with the specific operands of LHS reversed?
The assert is added in previous patch which is directly changing operands of operator Sub. Since that is a wrong patch and we create new node for RHS, I delete this assert.


https://reviews.llvm.org/D48754