[PATCH] Remove SCEVCache and FindConstantPointers from complete loop unrolling heuristic.

[Chandler Carruth]

This is looking really good. A bunch of minor stuff below. I'm happy for you to just address most of this before it gets committed, but I don't see the definition of 'simplifyUsingOffsets'. That's the only thing I really want to see before LGTM-ing.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:275
@@ +274,3 @@
+    Value *Base = nullptr;
+    Constant *Offset = nullptr;
+  };
----------------
I think this should just be a ConstantInt, it'll save you casting below.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:290-296
@@ -464,1 +289,9 @@
 private:
+  /// \brief A cache of pointer bases and constant-folded offsets corresponding
+  /// to GEP (or derived from GEP) instructions.
+  ///
+  /// In order to find the base pointer one needs to perform non-trivial
+  /// traversal of the corresponding SCEV expression, so it's good to have the
+  /// results saved.
+  DenseMap<Value *, SimplifiedAddress> SimplifiedOffsets;
+
----------------
I'd call this 'SimplifiedAddresses'. That also corresponds nicely to them being pointers.

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:340
@@ +339,3 @@
+
+    if (auto *AR = dyn_cast<SCEVAddRecExpr>(S)) {
+      const SCEV *ValueAtIteration =
----------------
I would invert this to use early exit.

  auto *AR = dyn_cast<SCEVAddRecExpr>(S);
  if (!AR)
    return false;
  
  ...

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:348-349
@@ +347,4 @@
+      }
+      // Check if the offset from the base address becomes a constant.
+      auto *Base = dyn_cast<SCEVUnknown>(SE.getPointerBase(S));
+      if (!Base)
----------------
Should we check that I is a pointer? Shouldn't this be dyn_cast_or_null?

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:356-357
@@ +355,4 @@
+        Address.Base = Base->getValue();
+        Address.Offset = dyn_cast<ConstantInt>(Offset->getValue());
+        assert(Address.Offset && "Offset is expected to be ConstantInt.");
+        SimplifiedOffsets[I] = Address;
----------------
If you're going to assert, you can just write 'cast<ConstantInt>' and it will assert for you.

I think that means you can just write:

  SimplifiedOffsets[I] = {Base->getValue(), cast<ConstantInt>(Offset->getValue())};


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:387
@@ +386,3 @@
+    if (!isa<Constant>(LHS) && !isa<Constant>(RHS))
+      if (!simplifyUsingOffsets(LHS, RHS))
+        return Base::visitBinaryOperator(I);
----------------
Where is this defined?

================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:410-412
@@ -520,7 +409,5 @@
 
-    auto *GEP = dyn_cast<GetElementPtrInst>(AddrOp);
-    if (!GEP)
-      return false;
-    auto OptionalGEPDesc = SC.getGEPDescriptor(GEP);
-    if (!OptionalGEPDesc)
+    if (!SimplifiedOffsets.count(AddrOp))
       return false;
+    SimplifiedAddress Address = SimplifiedOffsets.lookup(AddrOp);
+    ConstantInt *SimplifiedAddrOp = dyn_cast<ConstantInt>(Address.Offset);
----------------
I think I've mentioned this before, but please don't use 'count' and then 'lookup'. It queries the hashtable twice. You should just use 'find' and the iterator.

http://reviews.llvm.org/D10205

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/