[PATCH] D33942: [Polly] [ScopInfo] Do not use ScopStmt in Domain derivation of ScopInfo. NFC

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Analysis/ScopBuilder.cpp:956
 
-  scop->addUserAssumptions(AC, DT, LI);
+  scop->addUserAssumptions(AC, DT, LI, InvalidDomainMap);
 
----------------
The ScopStmt's invalid domains have already set in `buildDomains`. That is, the elements of `InvalidDomainMap` have been consumed already and setting them has no effect.


================
Comment at: lib/Analysis/ScopInfo.cpp:2146
     auto *Dom = InScop ? getDomainConditions(&Stmt) : isl_set_copy(Context);
-    bool Valid = buildConditionSets(Stmt, Val, TI, L, Dom, ConditionSets);
+    bool Valid = buildConditionSets(*this, Stmt.getBasicBlock(), Val, TI, L,
+                                    Dom, InvalidDomainMap, ConditionSets);
----------------
`getBasicBlock()` returns `nullptr` if `Stmt` is a region statement.


================
Comment at: lib/Analysis/ScopInfo.cpp:2668-2669
+  // Initialize the invalid domain.
+  for (ScopStmt &Stmt : Stmts)
+    Stmt.setInvalidDomain(InvalidDomainMap[Stmt.getEntryBlock()]);
+
----------------
When we have multiple statements per BasicBlock, this would consume the same `InvalidDomainMap[Stmt.getEntryBlock()]` twice, giving us an use-after-free sometime later.

How are you sure that all entries of  `InvalidDomainMap` are an entry block of some statement? There could be an BB from the middle of a region statement. Such a block's invalid domain would not be consumed, therefore leaking.


================
Comment at: lib/Analysis/ScopInfo.cpp:2853
   // Initialize the invalid domain.
-  ExitStmt->setInvalidDomain(isl_set_empty(isl_set_get_space(ExitDomain)));
+  InvalidDomainMap[ExitBB] = isl_set_empty(isl_set_get_space(ExitDomain));
 
----------------
You are leaking isl objects here: InvalidDomainMap[ExitBB] may already contain a value.

This is probably not the only leak. You can find some leak by using `valgrind --leak-check=full` on a failing test case with `-polly-on-isl-error-abort=0` added could be useful to find those.


https://reviews.llvm.org/D33942