This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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.

lib/Analysis/ScopInfo.cpp
2357–2360

Shouldn't this still be an error?

2377–2378

This is now superfluous.

2409

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

lib/Transform/FlattenAlgo.cpp
61

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.
lib/Analysis/ScopInfo.cpp
2357–2360

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?

2377–2378

Dropped.

2396

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.

lib/Transform/FlattenAlgo.cpp
61

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
lib/Analysis/ScopInfo.cpp
2396

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

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

lib/Transform/FlattenAlgo.cpp
61
[&](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.

lib/Analysis/ScopInfo.cpp
2396

I see. Seems we agree.

lib/Transform/FlattenAlgo.cpp
61

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?

lib/Analysis/ScopInfo.cpp
2357–2360

There is a clean-up of this in D45066.

2393

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

2404

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.

lib/Support/ISLTools.cpp
478–479

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

lib/Transform/ForwardOpTree.cpp
271

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

lib/Transform/Simplify.cpp
100–102

bool Aborted = false;

105–106

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

116

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

lib/Transform/ZoneAlgo.cpp
260–261

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.

870

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!