[PATCH] D152278: [SCEV] Compute SCEV for ashr(add(shl(x, n), c), m) instr triplet

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7893
+          // type, so create a new Constant with type same as TruncTy
+          int64_t AddOperand = AddOperandCI->getSExtValue();
+          const SCEV *AddConstant = getConstant(TruncTy, AddOperand >> AShrAmt);
----------------
Is there some reason to expect that "c" fits into an int64_t?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7894
+          int64_t AddOperand = AddOperandCI->getSExtValue();
+          const SCEV *AddConstant = getConstant(TruncTy, AddOperand >> AShrAmt);
+
----------------
I guess the reason AddOperandCI has to be a constant is that this shift needs to constant-fold?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:7897
+          // As of now only handle cases where n = m, and n should be a value
+          // corresponding to a 8, 16, 32 integer.
+          if (LShift->getOperand(1) == BO->RHS &&
----------------
This could be extended to cases where n is greater than m?  You can skip that for the initial patch, of course.

I don't really see any reason to restrict the shift amounts like this; the transform is pretty restricted even without that.  What effect are you worried about?


================
Comment at: llvm/test/Analysis/ScalarEvolution/sext-add-inreg.ll:37
+for.cond.cleanup:                                 ; preds = %for.body
+  ret i32 undef
+
----------------
Try to avoid "undef" in testcases where it isn't relevant.  I don't think it really has much effect here, but still.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152278