[PATCH] D18822: [Polly][FIX] Adjust execution context of hoisted loads wrt. error domains

[Michael Kruse via llvm-commits]

Meinersbur added a comment.

The summary does not mention this is supposed to fix PR26683 or what was wrong with hoisted loads before.
There's a typo: s/therefor/therefore


================
Comment at: include/polly/ScopInfo.h:1320
@@ -1319,1 +1319,3 @@
 
+  /// @brief A map from basic blocks to the error context reaching them.
+  DenseMap<BasicBlock *, isl_set *> ErrorDomainCtxMap;
----------------
Could you explain somewhere what an 'error context' is?

================
Comment at: include/polly/ScopInfo.h:1427-1441
@@ -1423,2 +1426,17 @@
 
+  /// @brief Propagate of error block domain contexts through @p R.
+  ///
+  /// This method will propagate error block domain contexts through @p R
+  /// and thereby fill the ErrorDomainCtxMap map. Additionally, the domains
+  /// of error statements and those only reachable via error statements will
+  /// be replaced by an empty set. Later those will be removed completely from
+  /// the SCoP.
+  ///
+  /// @param R  The currently traversed region.
+  /// @param SD The ScopDetection analysis for the current function.
+  /// @param DT The DominatorTree for the current function.
+  /// @param LI The LoopInfo for the current function.
+  void propagateErrorConstraints(Region *R, ScopDetection &SD,
+                                 DominatorTree &DT, LoopInfo &LI);
+
   /// @brief Propagate domains that are known due to graph properties.
----------------
Maybe this and the implementation could be moved to where removeErrorBlockDomains was to make clear it's a replacement.

================
Comment at: lib/Analysis/ScopInfo.cpp:2178-2185
@@ -2177,10 +2144,1 @@
 
-  // Error blocks and blocks dominated by them have been assumed to never be
-  // executed. Representing them in the Scop does not add any value. In fact,
-  // it is likely to cause issues during construction of the ScopStmts. The
-  // contents of error blocks have not been verfied to be expressible and
-  // will cause problems when building up a ScopStmt for them.
-  // Furthermore, basic blocks dominated by error blocks may reference
-  // instructions in the error block which, if the error block is not modeled,
-  // can themselves not be constructed properly.
-  removeErrorBlockDomains(SD, DT, LI);
----------------
The comment still applies for the most part, doesn't it?

================
Comment at: lib/Analysis/ScopInfo.cpp:2212-2223
@@ +2211,14 @@
+
+  ReversePostOrderTraversal<Region *> RTraversal(R);
+  for (auto *RN : RTraversal) {
+
+    // Recurse for affine subregions but go on for basic blocks and non-affine
+    // subregions.
+    if (RN->isSubRegion()) {
+      Region *SubRegion = RN->getNodeAs<Region>();
+      if (!SD.isNonAffineSubRegion(SubRegion, &getRegion())) {
+        propagateErrorConstraints(SubRegion, SD, DT, LI);
+        continue;
+      }
+    }
+
----------------
I have not better immediate solution but would like to remark the following:
- This pattern occurs multiple times (buildDomainsWithBranchConstraints, propagateDomainConstraints, propagateErrorConstraints) so it might make sense to wrap it.
- The comment of ReversePostOrderTraversal mentions it is expensive to create
- Why does it need to be recursive over nested regions, instead non-recursive over BB reverse post-order?


================
Comment at: lib/Analysis/ScopInfo.cpp:2242
@@ +2241,3 @@
+
+    if (!IsErrorBlock)
+      isl_set_free(DomainCtx);
----------------
else

================
Comment at: lib/Analysis/ScopInfo.cpp:3070-3071
@@ -3050,2 +3069,4 @@
   isl_set *DomainCtx = isl_set_params(Stmt.getDomain());
+  if (auto *ErrorCtx = getErrorCtxReachingStmt(Stmt))
+    DomainCtx = isl_set_subtract(DomainCtx, ErrorCtx);
   DomainCtx = isl_set_remove_redundancies(DomainCtx);
----------------
Isn't it enough just use the union of all error domains (or the context) instead per-stmt?

That domain could contain the hoisted value itself, but which could be just outprojected.

================
Comment at: test/ScopInfo/error-blocks-1.ll:10-20
@@ +9,13 @@
+;
+; CHECK:         Statements {
+; CHECK-NEXT:    	Stmt_if_end
+; CHECK-NEXT:            Domain :=
+; CHECK-NEXT:                [N] -> { Stmt_if_end[i0] : 0 <= i0 < N };
+; CHECK-NEXT:            Schedule :=
+; CHECK-NEXT:                [N] -> { Stmt_if_end[i0] -> [i0] };
+; CHECK-NEXT:            ReadAccess :=	[Reduction Type: +] [Scalar: 0]
+; CHECK-NEXT:                [N] -> { Stmt_if_end[i0] -> MemRef_A[i0] };
+; CHECK-NEXT:            MustWriteAccess :=	[Reduction Type: +] [Scalar: 0]
+; CHECK-NEXT:                [N] -> { Stmt_if_end[i0] -> MemRef_A[i0] };
+; CHECK-NEXT:    }
+;
----------------
Because there is no load hoisting, what dos this patch change in this test case?

================
Comment at: test/ScopInfo/error-blocks-2.ll:9
@@ +8,3 @@
+; CHECK-NEXT:                [N, valid_val] -> { Stmt_S[i0] -> MemRef_ptr_addr[0] };
+; CHECK-NEXT:            Execution Context: [N, valid_val] -> {  : N > 0 and (valid_val < 0 or valid_val > 0) }
+; CHECK-NEXT:            ReadAccess :=	[Reduction Type: NONE] [Scalar: 0]
----------------
Are the invariant accesses ordered? Is the previous invariant access for MemRef_valid guaranteed to be executed before this and to use valid_val ?


http://reviews.llvm.org/D18822