[PATCH] D20695: Floating Point SCEV Analysis

[Sanjoy Das via llvm-commits]

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