[PATCH] D20695: Floating Point SCEV Analysis
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Mon May 30 00:02:46 PDT 2016
sanjoy added a comment.
Hi Elena,
I have made some minor comments inline, but I still stand by my earlier comment that we should do something like this as a last resort. As an initial step we should at least evaluate how far we can we can get on relevant workloads without teaching SCEV about floating point values at all.
================
Comment at: ../include/llvm/Analysis/ScalarEvolutionExpressions.h:310
@@ +309,3 @@
+ ///
+ class SCEVFAddExpr : public SCEVCommutativeExpr {
+ friend class ScalarEvolution;
----------------
What is the intent behind making `SCEVFAddExpr` and `SCEVFMulExpr` subclasses of `SCEVNAryExpr`? Does `(a + b + c)` represent `((a + b) + c)` or `(a + (b + c))`?
================
Comment at: ../lib/Analysis/ScalarEvolution.cpp:1503
@@ +1502,3 @@
+ if (Start->getSCEVType() == scConstant) {
+ int64_t IntVal = cast<SCEVConstant>(Start)->getAPInt().getSExtValue();
+ StartFP = getFpConstant(Ty, (double)IntVal);
----------------
Note: `getSExtValue` will assert for integers that are large than 64 bits.
================
Comment at: ../lib/Analysis/ScalarEvolution.cpp:2544
@@ +2543,3 @@
+ while (const SCEVFAddExpr *Add = dyn_cast<SCEVFAddExpr>(Ops[Idx])) {
+ // If we have an add, expand the add operands onto the end of the operands
+ // list.
----------------
Depending on how we define the associativity of an `SCEVFAddExpr`, this may or may not be valid.
================
Comment at: ../lib/Analysis/ScalarEvolution.cpp:2875
@@ +2874,3 @@
+const SCEV *ScalarEvolution::getFMulExpr(SmallVectorImpl<const SCEV *> &Ops) {
+
+ assert(!Ops.empty() && "Cannot get empty mul!");
----------------
This is all duplicated code. If we go ahead with this, we should definitely common this with the integer version.
================
Comment at: ../lib/Analysis/ScalarEvolution.cpp:2895
@@ +2894,3 @@
+
+ // C1*(C2+V) -> C1*C2 + C1*V
+ if (Ops.size() == 2)
----------------
I thought floating point in general isn't distributive?
Repository:
rL LLVM
http://reviews.llvm.org/D20695
More information about the llvm-commits
mailing list