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

[Dominik Adamski via Phabricator via llvm-commits]

domada added inline comments.


================
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(),
----------------
Meinersbur wrote:
> 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`.
Yes, it was an intentional change. I wanted to move `getOrCreateScopArrayInfo` to ScopBuilder. Currently this function is used in lib/CodeGen/PPCGCodeGeneration.cpp and in ScopBuilder. That's why I haven't moved it in this patch.  I need to rethink this change.


================
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())) {
----------------
Meinersbur wrote:
> 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.
Thank you for your comment. Initially I will set `RecordedAssumption` to `nullptr`. Finally I will try to move Affinator into ScopBuilder. I think that it should be the most elegant way.


Repository:
  rPLO Polly

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

https://reviews.llvm.org/D68056