[PATCH] D37298: [ForwardOpTree] Allow forwarding in the presence of region statements

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/Analysis/ScopInfo.cpp:1740-1756
+bool ScopStmt::hasBoxedLoops() const {
+  Loop *L = getSurroundingLoop();
+
+  if (L) {
+    for (Loop *L : L->getSubLoops())
+      if (contains(L))
+        return true;
----------------
grosser wrote:
> Meinersbur wrote:
> > [Suggestion] Wouldn't is be easier to enumerate over `Scop::getBoxedLoops()` and check those whether they are contained in the stmt?
> This would grow linearly in the size of the scop, whereas the current approach hopefully would not.
You can check whether a given BB is in a boxed loop in constant time (assuming hashtable lookup is constant time):
```
LI.getLoopFor(BB) != Stmt.getSurroundingLoop()
```
I think closer to what is actually needed, no?


================
Comment at: lib/Transform/ZoneAlgo.cpp:379
     // might be in a boxed loop.
-    if (Stmt->isRegionStmt() &&
+    if (Stmt->hasBoxedLoops() &&
         !isl_union_map_is_disjoint(Loads.keep(), AccRel.keep())) {
----------------
grosser wrote:
> Meinersbur wrote:
> > [Serious] Boxed loops are not the only problematic case. Consider within a region stmt:
> > ```
> >   br label %StmtB
> > 
> > StmtA:
> >   %a= load %A
> >   br label %StmtC  
> > 
> > StmtB:
> >   store %A 
> >   br label %StmtA  
> > 
> > StmtC:
> >   ...
> > ```
> > Iteration order might be StmtA first, then StmtB, although StmtB is executed before StmtA. It should be rejected, but is not.
> OK. What would you suggest to check instead?
If you want to keep it easy, leave it as it is. It would only have an effect for load-forwarding from region stmts (not to region stmt, which seems to be the case you are interested in) (and DeLICM to some extent).
In contrast to my previous assumption this is possible without crossing a phi node: if the load to be forwarded is in the region's entry block (or in strange cases of irreducible loops)

If you want to make it work, you can check the following:
- Allow reads and writes in the entry block (if not part of a boxed loop which can be checked in constant time as show above; to also cover the case of an irreducible loop which is I am not sure is possible: check that all incoming edges are from outside the region; the point is only to ensure that the BB is not executed multiple times in the same statement instance) like it is done for block stmts (which never have boxed loops).
- Disallow any writes in non-entry blocks, as it is now for all BBs in region stmts.

For really advanced analysis, find a topological order of access execution order an iterator over that. (Keep in mind that there is no topological order if there is a loop). Can be rejected in that case. `getAccessesInOrder()` unfortunately is not advances enough for this.


================
Comment at: test/ForwardOpTree/forward_region.ll:10
+; bodyB_entry:
+;   if (undef)
+; body_true:
----------------
grosser wrote:
> Meinersbur wrote:
> > This condition does not correspond to the IR below.
> I just moved the old test case. Will cross-check.
Might be that I wasn't as accurate myself in the old test case that was probably created by myself. Problably started with an undef, but wasn't accepted by ScopDetection.


================
Comment at: test/ForwardOpTree/noforward_region.ll:1
-; RUN: opt %loadPolly -polly-optree -analyze < %s | FileCheck %s -match-full-lines
-;
----------------
grosser wrote:
> Meinersbur wrote:
> > Why is this deleted? There are still cases in regions that are not allowed.
> OK.
I just noticed that this was renamed, not deleted. Sorry for the unjustified comment.

However, I think we have no test case for forwarding a load from a region yet, which would be interesting.


https://reviews.llvm.org/D37298