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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1771
 
   if (auto *SA = dyn_cast<SCEVMulExpr>(Op)) {
     // zext((A * B * ...)<nuw>) --> (zext(A) * zext(B) * ...)<nuw>
----------------
`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?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:1800
+                               MulLHS->getAPInt().logBase2();
+            Type *TruncTy = IntegerType::get(getContext(), NewTruncBits);
+            assert(NewTruncBits > 0 && "Can't represent 2^N in 2^(N-1) bits.");
----------------
Let's s/TruncTy/NewTruncTy for consistency with NewTruncBits


================
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(
----------------
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`.


https://reviews.llvm.org/D48158