[PATCH] D70521: [InstCombine] Canonicalize widenable.conditions for ease of pattern matching (and cases)

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1978
+  {
+    // Canonicalize WC to left when compexity is equal (otherwise, it's already there)
+    if (match(Op1, m_Intrinsic<Intrinsic::experimental_widenable_condition>()) &&
----------------
It's hard for me to match this comment to real code. Maybe just say "Swap OP1 & Op2 if Op2 is WC and Op1 is not" or "Canonicalize WC to LHS if RHS is not WC"?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1993
+    Value *X, *Y;
+    // (WC & B) & A --> (A & B) & WC
+    // (B & WC) & A is canonicalized to previous
----------------
That will change evaluation order between A and B.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1994
+    // (WC & B) & A --> (A & B) & WC
+    // (B & WC) & A is canonicalized to previous
+    // A can't be WC due to canonicalize to LHS
----------------
This comment confused me a bit. My initial understanding was that next 'if' should handle this case . It took me a while to understand that it is actually handled by one of the previous 'if'. May be reformulate in terms:
A can't be WC because...
B can't be WC because...


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2003
+    // A & (WC & C) --> WC & (A & C)
+    // A & (B & WC) is canonicalized to previous
+    // A might be WC
----------------
??? C can't be WC because...???


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

https://reviews.llvm.org/D70521