[PATCH] D49382: [InstrSimplify] fold sdiv if two operands are negatived and non-overflow
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 16 09:16:05 PDT 2018
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
More information about the llvm-commits
mailing list