[PATCH] D33942: [Polly] [WIP] Getting a SCoP statement for each instruction. NFC

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: include/polly/ScopInfo.h:2514-2517
+  /// Return the ScopStmt an instruction belongs to, or nullptr if it
+  /// does not belong to any statement in this Scop.
+  ScopStmt *getStmtFor(Instruction *Inst) const;
+
----------------
Why did you move the location of this declaration?


================
Comment at: lib/Analysis/ScopInfo.cpp:1327
     Instruction *AccessVal = cast<Instruction>(Access->getAccessValue());
-    assert(Parent.getStmtFor(AccessVal) == this);
     assert(!ValueWrites.lookup(AccessVal));
----------------
As long as we did not remove `getStmtFor(Instruction*)`, you can keep the assertion.


================
Comment at: lib/Analysis/ScopInfo.cpp:2654-2657
+    if (Stmt.isRegionStmt())
+      Stmt.setInvalidDomain(InvalidDomainMap[Stmt.getRegion()->getEntry()]);
+    else
+      Stmt.setInvalidDomain(InvalidDomainMap[Stmt.getBasicBlock()]);
----------------
There is `ScopStmt::getEntryBlock()` which returns you the (Entry-)Block for the two cases.


================
Comment at: lib/Analysis/ScopInfo.cpp:2770
       unsigned NumConjucts = isl_set_n_basic_set(SuccInvalidDomain);
       SuccStmt->setInvalidDomain(SuccInvalidDomain);
 
----------------
You must replace all occurrences of `setInvalidDomain` to update `InvalidDomainMap` instead.


================
Comment at: lib/Analysis/ScopInfo.cpp:2782
-
-    Stmt->setInvalidDomain(InvalidDomain);
   }
----------------
Shouldn't this changed to update `InvalidDomainMap`?


================
Comment at: lib/Analysis/ScopInfo.cpp:3021
     auto *PredBBDom = getDomainConditions(PredBB);
-    auto *PredBBLoop = getStmtFor(PredBB)->getSurroundingLoop();
+    auto *PredBBLoop = getStmtFor(&PredBB->back())->getSurroundingLoop();
+
----------------
The `StmtMap` does not contain all instructions. It never includes the terminator which `PredBB->back()` return. That is `getStmtFor` cannot any statement for its input.

This applies similarly to `getStmtFor(&BB->front()` as well which can be a synthesizable value.


https://reviews.llvm.org/D33942