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

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for this patch. It seems you guys already discussed this topic a lot and you seem to converge on the technical review. This is great. I will look at the updated version than once again.

Regarding this patch, it would be good to describe briefly in the commit message the original issue (at best with an example), the solution, and the reason alternatives have not been chosen. With this information one can understand this patch without having to dig through the previous discussion. Most likely you can just take some of the text that was in the bug report to discuss this.

One alternative that I learned about in the bug report was to revert an earlier patch (which svn id?). This would have fixed this bug here, but would have caused unnecessarily complicated domains. Right? Can we give a motivation why this would not be the case with the current approach?

s/priviously/previously/g


================
Comment at: lib/Analysis/ScopInfo.cpp:3060
@@ -3046,1 +3059,3 @@
 
+isl_set *Scop::getErrorCtxReachingStmt(ScopStmt &Stmt) {
+  auto *BB = Stmt.getEntryBlock();
----------------
Add __isl_give here as well.

================
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:    }
+;
----------------
jdoerfert wrote:
> Meinersbur wrote:
> > Because there is no load hoisting, what dos this patch change in this test case?
> This is a test for the removal of error statements that was poorly covered before.
If this test case is not changed / affected by this commit, I would suggest to commit it ahead of time.


http://reviews.llvm.org/D18822