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

[Matt Devereau via Phabricator via llvm-commits]

MattDevereau added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1006
+  auto Shift = cast<OverflowingBinaryOperator>(Op0);
+  if (S1 >= S2 && ((IsSigned && Q.IIQ.hasNoSignedWrap(Shift)) ||
+                   (!IsSigned && Q.IIQ.hasNoUnsignedWrap(Shift))))
----------------
david-arm wrote:
> Perhaps you can create a temp variable here to reuse the logic, i.e.
> 
>   bool NoWrap = (IsSigned && Q.IIQ.hasNoSignedWrap(Shift)) ||
>     (!IsSigned && Q.IIQ.hasNoUnsignedWrap(Shift));
>   if (S1 >= S2 && NoWrap)
>     return Constant::getNullValue(Shift->getType());
> 
>   if (NoWrap)
>     return Op0;
I'm not sure this suggestion is correct, they key thing here being `Shift = cast<OverflowingBinaryOperator>(Op1);` on line 1010 which changes `Shift` to be a cast of Op1 instead of Op0 on which the nsw/nuw flags are checked, which is because the larger shift value needs its flags checking. If we went ahead with this, in this example (https://alive2.llvm.org/ce/z/YiFELt) NoWrap would evaluate to true while the transform is not viable.


================
Comment at: llvm/test/Transforms/InstSimplify/rem.ll:502
+
+define i8 @neg_urem_shl(i8 %x){
+; CHECK-LABEL: @neg_urem_shl(
----------------
david-arm wrote:
> Could you leave comments on the negative tests explaining why they fail? If it makes it easier you could put all the negative tests together?
Sure thing, should be simple enough


================
Comment at: llvm/test/Transforms/InstSimplify/rem.ll:539
+
+define i8 @srem_shl(i8 %x){
+; CHECK-LABEL: @srem_shl(
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142901