[PATCH] D68056: [Polly][NFC][ScopBuilder] Move RecordedAssumptions vector to ScopBuilder.

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Nice one.



================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:1494-1495
 void ScopBuilder::addRecordedAssumptions() {
-  for (auto &AS : llvm::reverse(scop->recorded_assumptions())) {
+  for (auto &AS : llvm::reverse(make_range(RecordedAssumptions.begin(),
+                                           RecordedAssumptions.end()))) {
 
----------------
[style] `llvm::reverse(RecordedAssumptions)` should work as well.


================
Comment at: polly/lib/Analysis/ScopBuilder.cpp:1525
   }
-  scop->clearRecordedAssumptions();
+  RecordedAssumptions.clear();
 }
----------------
[suggestion] Now that `RecordedAssumptions` is part of ScopBuilder, it will be freed together with `ScopBuilder`, so explicitly clearing it should not be necessary.


================
Comment at: polly/lib/Analysis/ScopInfo.cpp:699-700
     Outside = Outside.gist_params(Statement->getDomain().params());
-  Statement->getParent()->recordAssumption(INBOUNDS, Outside, Loc,
-                                           AS_ASSUMPTION);
 }
----------------
Nice to have this moved out of `MemoryAccess`


================
Comment at: polly/lib/Analysis/ScopInfo.cpp:1351
 
-  ScopArrayInfo *SAI =
-      Parent.getOrCreateScopArrayInfo(V, V->getType(), {}, MemoryKind::Value);
+  ScopArrayInfo *SAI = Parent.getScopArrayInfo(V, MemoryKind::Value);
   Access = new MemoryAccess(this, nullptr, MemoryAccess::READ, V, V->getType(),
----------------
Is this an intentional change? Depending on the iteration order over basic blocks, the value might be used before its write is seem, in which case we'd need `getOrCreateScopArrayInfo`.


================
Comment at: polly/lib/Analysis/ScopInfo.cpp:1693
       R(R), name(None), HasSingleExitEdge(R.getExitingBlock()), DC(DC),
-      ORE(ORE), Affinator(this, LI),
+      ORE(ORE), Affinator(this, LI, RecordedAssumptions),
       ID(getNextID((*R.getEntry()->getParent()).getName().str())) {
----------------
The `RecordedAssumptions` stored inside `ScopBuilder` has a shorter lifetime than the `Affinator` stored inside this `Scop` object. Meaning that any use of `Affinator` after the destruction of `ScopBuilder` will lead to a use-after-free error. It should not happen since adding an assumption after the Scop has been constructed will have no effect. Now new assumptions should be detectable using Valgrind/AddressSanitizer, which might be a good thing, but the potential memory corruption worries me.

Do you have any plans how to ensure no use-after-free happens? E.g. set the `RecordedAssumption` to `nullptr` after the Scop has been built. We might even work on even moving the Affinator into ScopBuilder. I would at least appreciate a comment in the code explaining this.


================
Comment at: polly/lib/Support/ScopHelper.cpp:155-162
+void polly::recordAssumption(polly::RecordedAssumptionsTy &RecordedAssumptions,
+                             polly::AssumptionKind Kind, isl::set Set,
+                             DebugLoc Loc, polly::AssumptionSign Sign,
+                             BasicBlock *BB) {
+  assert((Set.is_params() || BB) &&
+         "Assumptions without a basic block must be parameter sets");
+  RecordedAssumptions.push_back({Kind, Sign, Set, Loc, BB});
----------------
[serious] The comment above applies to `simplifyRegion`, not `recordAssumption`.


Repository:
  rPLO Polly

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68056/new/

https://reviews.llvm.org/D68056