Page MenuHomePhabricator

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

Authored by grosser on Apr 28 2018, 11:14 AM.



isl::stat::error indicates a run-time error. We should not abuse it to
enforce an early abort of the foreach call. We should soon see a new
isl_*_every function family, which allows for specific return values to
abort early. When these are available, we should introduce these
deliberately to indicate early aborts.

In same cases we also abused these return values to indicate if a
certain value has been found or not. To avoid this abuse an extra
boolean is introduced to track this state.

Event Timeline

grosser created this revision.Apr 28 2018, 11:14 AM
grosser updated this revision to Diff 144459.Apr 28 2018, 11:28 AM

Convert two more functions

I think this makes sense, with some inline nits.


Shouldn't this still be an error?


This is now superfluous.


In case of isl_error_quota, this should also return false in that case.


You could check this condition before testing the BMap's out-dimension to save a little bit of compute.

grosser marked an inline comment as done.Apr 29 2018, 12:25 AM
grosser added inline comments.

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?




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.


I don't get what you have in mind here. Could you provide a mini code snippet?

grosser updated this revision to Diff 144472.Apr 29 2018, 12:29 AM
grosser marked an inline comment as done.

Address some of Philips comments

  • Remove unneeded return
philip.pfaffe added inline comments.Apr 29 2018, 4:11 AM

We do it today, courtesy of buildMinMaxAccess propagating the error.

But if we're dropping that propagation alltogether, then this is ceretainly fine.

[&](isl::basic_map BMap) -> isl::stat {
    if (IsVariableDim)
        return isl::stat::ok;


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.


I see. Seems we agree.


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.

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?


There is a clean-up of this in D45066.


buildMinMaxAccess returning isl::stat::error is an error: The access bounds could not be computed. It's no abuse.


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.


If UMap is NULL (or cannot be iterated over), this now returns an empty map instead of nullptr.


The // break comment should be moved to the line above.


bool Aborted = false;


Consistency: if (Aborted) added to this foreach, but not the following.


Resetting Aborted to false is unnecessary: Once Result has exceeded SimplifyMaxDisjuncts, no more disjuncts will be added.


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.


Like above, if UMap is NULL, this now returns true instead of false

I'm generally fine with this, just one more nit: I don't like for the argument to be called Aborted, for that suggests an error condition. What about BreakIteration instead?

Scop::containsExtensionNode is missing

Thanks. I am currently upstreaming a set of functions to isl which allow us to get basic_map_lists from a map and map_lists from a union_map. For isl_*_list types it should be easy to write an iterator that allows foreach loops. I plan to write such iterators and will then use these to simplify this code.

grosser abandoned this revision.Jun 1 2018, 11:03 AM

Dropping this in favor of D47604. Thanks for the suggestions!