[PATCH] D142901: [InstSimplify] Simplify UREM and SREM left shifted operands

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau marked 3 inline comments as done.
MattDevereau added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:995
 
+Value *simplifyRemShifts(Value *Op0, Value *Op1, bool IsSigned,
+                         const SimplifyQuery &Q) {
----------------
goldstein.w.n wrote:
> Instead of a new helper maybe this should be in `simplifyRem` which already appears to have:
> ```
>   // (X << Y) % X -> 0
>   if (Q.IIQ.UseInstrInfo &&
>       ((Opcode == Instruction::SRem &&
>         match(Op0, m_NSWShl(m_Specific(Op1), m_Value()))) ||
>        (Opcode == Instruction::URem &&
>         match(Op0, m_NUWShl(m_Specific(Op1), m_Value())))))
>     return Constant::getNullValue(Op0->getType());
> ```
> 
> This is really just a superset of that case, so maybe expanding the existing logic would be simpler?
I've inlined it into `simplifyRem` which I think looks a lot neater now. Thank you for the feedback.


================
Comment at: llvm/test/Transforms/InstSimplify/rem.ll:539
+
+define i8 @srem_shl(i8 %x){
+; CHECK-LABEL: @srem_shl(
----------------
david-arm wrote:
> MattDevereau wrote:
> > david-arm wrote:
> > > Is it worth having a negative test for `srem` with `nuw` shifts as well? We shouldn't do any simplification in that case either.
> > I personally think the current tests are ok since we have one test for nsw which is testing the presence of nsw, and one which tests for it's absence and other flags aren't particularly relevant. These tests are quite compact though so I suppose there's no harm in adding extra cases.
> Sure, I was just thinking that specifically we don't want to perform the simplification for any of nsw or nuw. They both mark the instruction as not wrapping, but only the signed version applies here. It's just because your logic accepts both nsw and nuw flags as valid, but *only if* the signedness matches the instruction. I was hoping to defend against someone accidentally rewriting your code in future and losing the signedness checks.
I've added negative tests for urem with nsw and srem with nuw now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142901