[PATCH] D89317: [InstructionSimplify] icmp simplification

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:2870-2871
+  // Canonicalize nsw add as RHS.
+  if (!match(RHS, m_NSWAdd(m_Value(), m_Value())))
+    std::swap(LHS, RHS);
+  if (!match(RHS, m_NSWAdd(m_Value(), m_Value())))
----------------
SjoerdMeijer wrote:
> spatel wrote:
> > I think this works, but I'm worried that it could break if we are not careful.
> > If we are swapping the operands, should we also swap the predicate, and then check if it is SLT after the swap?
> Yep, more predicates could be supported here, but I thought to have a start with SLT first, hence the `Pred != CmpInst::ICMP_SLT` check above on line 2866, so should be fine for now?
I think it's correct:
https://rise4fun.com/Alive/gyqW

So it's a question of what we mean when we say "canonicalize" in the code comment. Transforming the icmp so that the operands are swapped implies that the predicate is also swapped.

Either way, this patch is not complete as-is even for the SLT predicate alone. We miss this:
https://rise4fun.com/Alive/VgB

So we should note that with another TODO comment.


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

https://reviews.llvm.org/D89317