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

[Tobias Grosser via Phabricator via llvm-commits]

grosser marked an inline comment as done.
grosser added inline comments.


================
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;
----------------
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?



================
Comment at: lib/Analysis/ScopInfo.cpp:2380
 
-  return isl::stat::ok;
+  return;
 }
----------------
philip.pfaffe wrote:
> This is now superfluous.
Dropped.


================
Comment at: lib/Analysis/ScopInfo.cpp:2397
-  return Locations.foreach_set(Lambda) == isl::stat::ok;
+  Locations.foreach_set(Lambda);
+
+  return !Aborted;
 }
 
----------------
Why? We don't do this today either.

As the global quota state is already set to quota-error anything we will compute inside the quota guard will anyhow discarded. There is no need to communicate on any other pass that things are invalid and we should probably not even try as otherwise we would need to check for compute out's all over the place, if we would like to be consistent. As this is not possible, the intend is always that there is a final check that drops everything, if there was a compute-out error.


================
Comment at: lib/Transform/FlattenAlgo.cpp:61
     if (isVariableDim(BMap))
-      return isl::stat::error;
+      IsVariableDim = true;
     return isl::stat::ok;
----------------
philip.pfaffe wrote:
> You could check  this condition before testing the BMap's out-dimension to save a little bit of compute.
I don't get what you have in mind here. Could you provide a mini code snippet?


Repository:
  rPLO Polly

https://reviews.llvm.org/D46227