[PATCH] D46227: [islpp] Do not abuse isl::stat::error as early-abort

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Why not just waiting until `isl_*_every` is available? When it comes, do we need to undo the changes here again?

Did you consider adding a wrapper `foreach` that sets/checks the `Aborted` flag itself?



================
Comment at: lib/Analysis/ScopInfo.cpp:2393
-  auto Lambda = [&MinMaxAccesses, &S](isl::set Set) -> isl::stat {
-    return buildMinMaxAccess(Set, MinMaxAccesses, S);
   };
----------------
`buildMinMaxAccess` returning `isl::stat::error` **is** an error: The access bounds could not be computed. It's no abuse.


================
Comment at: lib/Analysis/ScopInfo.cpp:2404
+  auto Lambda = [&MinMaxAccesses, &S, &Aborted](isl::set Set) -> isl::stat {
+    buildMinMaxAccess(Set, MinMaxAccesses, S, Aborted);
+    return isl::stat::ok;
----------------
Instead of making `buildMinMaxAccess` handle the "`break;`" of a loop that is called in, I'd prefer if it is handled directly in the lambda.


================
Comment at: lib/Analysis/ScopInfo.cpp:2358
 
-  if (isl_ctx_last_error(Ctx.get()) == isl_error_quota)
-    return isl::stat::error;
+  if (isl_ctx_last_error(Ctx.get()) == isl_error_quota) {
+    Aborted = true;
----------------
grosser wrote:
> philip.pfaffe wrote:
> > Shouldn't this still be an error?
> I don't think this is needed. Isl's global error state is already set to 'error'. I don't see which benefit it has to pass this fact _again_ to isl. Also, we check for this fact again at:
> 
> ```  for (AliasGroupTy &AG : AliasGroups) {                                         
>     if (!hasFeasibleRuntimeContext())                                            
>       return false;                                                              
>                                                                                  
>     {                                                                            
>       IslMaxOperationsGuard MaxOpGuard(getIslCtx().get(), OptComputeOut);        
>       bool Valid = buildAliasGroup(AG, HasWriteAccess);                          
>       if (!Valid)                                                                
>         return false;                                                            
>     }                                                                            
>     if (isl_ctx_last_error(getIslCtx().get()) == isl_error_quota) {              
>       invalidate(COMPLEXITY, DebugLoc());                                        
>       return false;                                                              
>     }  
> ```
> 
> In fact, I feel we should probably just drop this condition entirely. Not sure why it even has been left here. 
> 
> @Meinersbur, any idea?
> 
There is a clean-up of this in D45066.


================
Comment at: lib/Support/ISLTools.cpp:478-479
   });
-  if (Success != isl::stat::ok)
-    return {};
   return Result;
----------------
If `UMap` is `NULL` (or cannot be iterated over), this now returns an empty map instead of `nullptr`.


================
Comment at: lib/Transform/ForwardOpTree.cpp:271
+      Aborted = true;
+      return isl::stat::ok; // break
     });
----------------
The `// break` comment should be moved to the line above.


================
Comment at: lib/Transform/Simplify.cpp:100-102
+  bool Aborted;
+
+  Aborted = false;
----------------
`bool Aborted = false;`


================
Comment at: lib/Transform/Simplify.cpp:105-106
+      [&Result, &Aborted](isl::basic_map BMap) -> isl::stat {
+        if (Aborted)
+          return isl::stat::ok;
+
----------------
Consistency: `if (Aborted)` added to this foreach, but not the following.


================
Comment at: lib/Transform/Simplify.cpp:116
+
+  Aborted = false;
+  Map.foreach_basic_map([&Result, &Aborted](isl::basic_map BMap) -> isl::stat {
----------------
Resetting `Aborted` to false is unnecessary: Once `Result` has exceeded `SimplifyMaxDisjuncts`, no more disjuncts will be added.


================
Comment at: lib/Transform/ZoneAlgo.cpp:260-261
   });
-  if (Success != isl::stat::ok)
-    return {};
   return Result;
----------------
There's a difference in semantics here: If `UMap` is `NULL` (or cannot be iterated over), this function would return `nullptr`, instead of an empty map.


================
Comment at: lib/Transform/ZoneAlgo.cpp:870
   });
-  return Result == isl::stat::ok;
+  return IsNormalized;
 }
----------------
Like above, if `UMap` is NULL, this now returns `true` instead of `false`


Repository:
  rPLO Polly

https://reviews.llvm.org/D46227