[PATCH] D149421: [KnownBits] Improve `KnownBits::rem(X, Y)` in cases where we can deduce low-bits of output

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Looks reasonable overall.



================
Comment at: llvm/lib/Support/KnownBits.cpp:552
+  if (!RHS.isZero() && RHS.Zero[0]) {
+    // rem X, Y where Y[0:N] is zero will X[0:N] in the result.
+    unsigned RHSZeros = RHS.countMinTrailingZeros();
----------------
Missing word after "will"?


================
Comment at: llvm/lib/Support/KnownBits.cpp:554
+    unsigned RHSZeros = RHS.countMinTrailingZeros();
+    APInt Mask = APInt::getBitsSet(LHS.getBitWidth(), 0, RHSZeros);
+    APInt OnesMask = LHS.One & Mask;
----------------
Use `getLowBitsSet`?


================
Comment at: llvm/lib/Support/KnownBits.cpp:557
+    APInt ZerosMask = LHS.Zero & Mask;
+    assert((OnesMask & ZerosMask).isZero());
+    Known.One |= OnesMask;
----------------
We already checked `!LHS.hasConflict()` in both callers so this seems unnecessary to me.


================
Comment at: llvm/lib/Support/KnownBits.cpp:576
 
+  remGetLowBits(Known, LHS, RHS);
+
----------------
Just a matter of taste, but I'd prefer to write this as `Known |= remGetLowBits(LHS, RHS);`.

Also could move this above the `RHS.isConstant` case - then that case would only need to handle setting the high bits of Known.Zero.


================
Comment at: llvm/test/Analysis/ScalarEvolution/merge-add-rec-many-inputs.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
 ; RUN: opt < %s -disable-output "-passes=print<scalar-evolution>" 2>&1 | FileCheck %s
----------------
Looks like a spurious change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149421