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

[Michael Zolotukhin]

Hi Chandler,

Thanks for the review! I'll address the concerns and commit the patch soon. Please also find some comments inline:


================
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)
----------------
chandlerc wrote:
> Should we check that I is a pointer? Shouldn't this be dyn_cast_or_null?
No, this is correct.
`getPointerBase(S)` returns a SCEV expression - if it fails to find the base, it'll return the incoming SCEV expression `S`. Then, when we try to cast it to `SCEVUnknown` we get `nullptr`, which is tested in the following check.
So, the fail condition here is getting not `SCEVUnknown`.

================
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);
----------------
chandlerc wrote:
> Where is this defined?
Oops, it crept to the subsequent patch (D10206). I'll move it to this one before committing, but do you mind taking a glance there too?

http://reviews.llvm.org/D10205

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