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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Nice improvement @MattDevereau! I just had a few minor 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))))
----------------
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;


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


================
Comment at: llvm/test/Transforms/InstSimplify/rem.ll:539
+
+define i8 @srem_shl(i8 %x){
+; CHECK-LABEL: @srem_shl(
----------------
Is it worth having a negative test for `srem` with `nuw` shifts as well? We shouldn't do any simplification in that case either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142901