[PATCH] D13195: [Polly] Allow invariant loads in the SCoP description

[Tobias Grosser via llvm-commits]

grosser accepted this revision.
grosser added a comment.
This revision is now accepted and ready to land.

LGTM.


================
Comment at: include/polly/ScopDetection.h:145
@@ -143,3 +144,3 @@
 
   /// @brief Map to remeber loops in non-affine regions.
   using BoxedLoopsMapTy = DenseMap<const Region *, BoxedLoopsSetTy>;
----------------
remember (unrelated, but could be fixed as well)

================
Comment at: include/polly/ScopInfo.h:1219
@@ -1215,2 +1218,3 @@
+  /// @brief Hoist invariant memory loads and check for required ones.
   void hoistInvariantLoads();
 
----------------
This is very brief. It might be a good place to put an overview (and some examples of your approach).

================
Comment at: include/polly/Support/ScopHelper.h:47
@@ +46,3 @@
+/// @brief Type for a set of invariant loads.
+using InvariantLoadsSetTy = llvm::SetVector<llvm::LoadInst *>;
+
----------------
We should use an AssertingVH<LoadInst> to ensure we do not have dangling pointers.

================
Comment at: lib/Analysis/ScopDetection.cpp:694
@@ -684,2 +693,3 @@
       // sure the base pointer is not an instruction defined inside the scop.
+      // However, if we can ignore loads that will be hoisted.
       for (const auto &Ptr : AS) {
----------------
, we can

================
Comment at: lib/Analysis/ScopDetection.cpp:698
@@ -687,1 +697,3 @@
         if (Inst && CurRegion.contains(Inst)) {
+          LoadInst *LInst = dyn_cast<LoadInst>(Inst);
+          if (LInst && isHoistableLoad(LInst, CurRegion, *LI, *SE)) {
----------------
You can use 'auto' here.

Also, "Load' instead of "LInst" seams easier to read.

================
Comment at: lib/Analysis/ScopDetection.cpp:1132
@@ +1131,3 @@
+ScopDetection::getRequiredInvariantLoads(const Region *R) const {
+  auto RILMIt = RequiredInvariantLoadsMap.find(R);
+  if (RILMIt == RequiredInvariantLoadsMap.end())
----------------
RILMIt? Just I seems fine for an iterator variable with minimal scop. it is easier to remember and as understandable as RILMIt.

================
Comment at: lib/Analysis/ScopInfo.cpp:1405
@@ +1404,3 @@
+  // we will hoist. Otherwise we would have cyclic dependences on the
+  // constraints under which the hoisted loads are executed.
+  for (MemoryAccess *MA : InvMAs) {
----------------
A little example would make this a bit more understandable.

================
Comment at: lib/Analysis/ScopInfo.cpp:2460
@@ +2459,3 @@
+      DEBUG(errs() << "\n\nWARNING: Load (" << *LI
+                   << ") is required to be invariant but was not marked as "
+                      "such. SCoP for "
----------------
errs() -> dbgs()

================
Comment at: lib/Support/ScopHelper.cpp:16
@@ -15,3 +15,2 @@
 #include "polly/ScopInfo.h"
-#include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/LoopInfo.h"
----------------
Unrelated.

================
Comment at: test/ScopInfo/invariant_load_ptr_ptr_noalias.ll:4
@@ -3,1 +3,3 @@
 ;
+; Note: The order of the invariant accesses is important!
+;
----------------
Maybe explain why the order matters.


http://reviews.llvm.org/D13195