[PATCH] D49382: [InstrSimplify] fold sdiv if two operands are negatived and non-overflow

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a reviewer: lebedev.ri.
spatel added a comment.

Should we generalize this, so we can include 'srem' (doesn't need 'nsw')?
https://rise4fun.com/Alive/tme

It's probably better to make that a separate patch in any case.

I'm going to take a break for a few days, so I may not be able to continue the review promptly. Adding Roman as a potential reviewer.



================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1087
 
 Value *llvm::SimplifySDivInst(Value *Op0, Value *Op1, const SimplifyQuery &Q) {
+  // If two operands are negation and no signed overflow, return -1.
----------------
This is not the right place to add logic. It should go in here instead:
static Value *SimplifySDivInst()
...because that is called internally when the external user makes a call to llvm::SimplifyBinOp()


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1092-1100
+    // Op0 is no signed wrap.
+    if (match(Op0, m_Sub(m_Value(), m_Value())))
+      nsw &= (cast<OverflowingBinaryOperator>(Op0))->hasNoSignedWrap();
+
+    // Op1 is no signed wrap.
+    if (match(Op1, m_Sub(m_Value(), m_Value())))
+      nsw &= (cast<OverflowingBinaryOperator>(Op1))->hasNoSignedWrap();
----------------
I don't think this behaves as you're expecting. Please add a test like this:

```
define i32 @irrelevant_sub(i32 %t) {
  %x = sub i32 %t, 5
  %negx = sub nsw i32 0, %x
  %div = sdiv i32 %x, %negx
  ret i32 %div
}

```

It would be better to match the patterns with m_Neg separately from the case with (x-y)/(y-x).


https://reviews.llvm.org/D49382