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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142901