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

[Michael Kruse via llvm-commits]

Meinersbur added inline comments.

================
Comment at: lib/Analysis/ScopInfo.cpp:2242
@@ +2241,3 @@
+
+    if (!IsErrorBlock)
+      isl_set_free(DomainCtx);
----------------
jdoerfert wrote:
> Meinersbur wrote:
> > else
> I did this on purpose but I can do else too.
Please do; I was looking for a leak for the case `IsErrorBlock == true` when I noticed that it's the negated condition.

================
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);
----------------
jdoerfert wrote:
> Meinersbur wrote:
> > 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.
> > Isn't it enough just use the union of all error domains (or the context) instead per-stmt?
> That would be an overapproximation that would basically make error statements and invariant loads "exclusive" again. Thus, the domain of a error-block could not depend on an invariant load, now it can.
> 
> > That domain could contain the hoisted value itself, but which could be just outprojected.
> I do not get this, sorry.
> That would be an overapproximation that would basically make error statements and invariant loads "exclusive" 
> again. Thus, the domain of a error-block could not depend on an invariant load, now it can.

OK, I see. Any ErrorDomainCtxMap is not only valid in the scop context, but even required for checking the context.

But in case of "union of all error domains", the validity check is a conjunction of all error domains. If one of them is is true, the other ones can be just `undef` and not influence the outcome.

At a second though, I think we'd have a problem with the evaluation order. One error domain needs to be checked first and might be only allowed to be dereferenced depending on another error domain.


http://reviews.llvm.org/D18822