[PATCH] D48158: [SCEV] Simplify zext/trunc idiom that appears when handling bitmasks.

[Justin Lebar via Phabricator via llvm-commits]

jlebar marked 3 inline comments as done.
jlebar added a comment.

Thank you for the reviews, Sanjoy!

Will push with your suggested changes.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1771
 
   if (auto *SA = dyn_cast<SCEVMulExpr>(Op)) {
     // zext((A * B * ...)<nuw>) --> (zext(A) * zext(B) * ...)<nuw>
----------------
sanjoy wrote:
> `SA` is a bad name for a `SCEVMulExpr` -- should be `SM` or `Mul`.  Do you mind fixing this in a followup CL while you're here?
Will do.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1801
+            Type *TruncTy = IntegerType::get(getContext(), NewTruncBits);
+            assert(NewTruncBits > 0 && "Can't represent 2^N in 2^(N-1) bits.");
+            return getMulExpr(
----------------
sanjoy wrote:
> Super minor issue - if `NewTruncBits` < `1` then `IntegerType::get` would have asserted already.  If you want to keep this assert for the better error message it is probably better to put it before `IntegerType::get`.
Sure, let's just remove this assert.


https://reviews.llvm.org/D48158