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

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Thanks @philip.pfaffe . Is this fine from your side, otherwise?

I am still waiting for feedback from Michael Kruse regarding dropping the compute-out stuff. Please feel free to comment on this as well.



================
Comment at: lib/Analysis/ScopInfo.cpp:2397
-  return Locations.foreach_set(Lambda) == isl::stat::ok;
+  Locations.foreach_set(Lambda);
+
+  return !Aborted;
 }
 
----------------
philip.pfaffe wrote:
> grosser wrote:
> > 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.
> We do it today, courtesy of `buildMinMaxAccess` propagating the error.
> 
> But if we're dropping that propagation alltogether, then this is ceretainly fine.
I see. Seems we agree.


================
Comment at: lib/Transform/FlattenAlgo.cpp:61
     if (isVariableDim(BMap))
-      return isl::stat::error;
+      IsVariableDim = true;
     return isl::stat::ok;
----------------
philip.pfaffe wrote:
> grosser wrote:
> > 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?
> ```
> [&](isl::basic_map BMap) -> isl::stat {
>     if (IsVariableDim)
>         return isl::stat::ok;
> 
>     ...
> }
> ```
I understand. I feel this adds just noise. I doubt we will save anything measurable. Would prefer to add it only if we can measure a perf difference.


Repository:
  rPLO Polly

https://reviews.llvm.org/D46227