[PATCH] D28308: [SCEV] Model ashr(shl(x, n), m) as mul(x, 2^(n-m)) when n > m

[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 modulo minor comments



================
Comment at: lib/Analysis/ScalarEvolution.cpp:5322
+        break;
+
+      Type *SExtTy = BO->LHS->getType();
----------------
I'd call this `OuterTy`, since at this point it isn't obvious what this has to do with `sext`.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:5336
+      uint64_t AShrAmt = CI->getZExtValue();
+      uint64_t TruncToWidth = BitWidth - AShrAmt;
+      Type *TruncTy = IntegerType::get(getContext(), TruncToWidth);
----------------
IMO things would read a bit clearer if you just s/`TruncToWidth`/`BitWidth - AShrAmt`/ everywhere.  But this is a minor stylistic thing, and I'd understand if you did not want to make this change.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:5346
+        if (L->getOperand(1) == BO->RHS)
+          // For a two-shift sext-inreg, i.e. n = m,
+          // use sext(trunc(x)) as the SCEV expression.
----------------
If you want to use `n` and `m`, please denote that in the legend above, i.e:

```
// X = Shl A, n
// Y = AShr X, m
```

or use `C0` and `C1` in this comment.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:5357
+            // expression. We already checked that ShlAmt < BitWidth, so
+            // the multiplier, 1 << (LhlAmt - AShrAmt), fits into TruncTy as
+            // LhlAmt - AShrAmt < Amt.
----------------
Typo: `LhlAmt`.


================
Comment at: test/Analysis/ScalarEvolution/sext-mul.ll:8
+; CHECK: %tmp11 = getelementptr inbounds i32, i32* %arg, i64 %tmp10
+; CHECK: --> {{.*}} Exits: ((4 * (sext i32 (-2 + (2 * %arg2)) to i64)) + %arg)		LoopDispositions: { %bb7: Computable }
+; CHECK:  %tmp14 = or i64 %tmp10, 1
----------------
Why not `CHECK-NEXT` here?


================
Comment at: test/Analysis/ScalarEvolution/sext-zero.ll:6
+; CHECK-NEXT:  %tmp10 = ashr exact i64 %tmp9, 0
+; CHECK-NEXT:  -->  {0,+,8589934592}<%bb7> U: [0,-8589934591) S: [-9223372036854775808,9223372028264841217)		Exits: (-8589934592 + (8589934592 * (zext i32 %arg2 to i64)))		LoopDispositions: { %bb7: Computable }
+
----------------
Please remove the `S: [-9223372036854775808,9223372028264841217)    Exits: (-8589934592 + (8589934592 * (zext i32 %arg2 to i64)))    LoopDispositions: { %bb7: Computable }` bits, here and elsewhere, unless you specifically want to check them.  They're pretty distracting.


Repository:
  rL LLVM

https://reviews.llvm.org/D28308