[polly] r292213 - Relax assert when setting access functions with invariant base pointers

[Tobias Grosser via llvm-commits]

On Tue, Jan 17, 2017, at 07:18 PM, Friedman, Eli via llvm-commits wrote:
> On 1/17/2017 4:00 AM, Tobias Grosser via llvm-commits wrote:
> > Author: grosser
> > Date: Tue Jan 17 06:00:42 2017
> > New Revision: 292213
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=292213&view=rev
> > Log:
> > Relax assert when setting access functions with invariant base pointers
> >
> > Summary:
> > Instead of forbidding such access functions completely, we verify that their
> > base pointer has been hoisted and only assert in case the base pointer was
> > not hoisted.
> >
> > I was trying for a little while to get a test case that ensures the assert is
> > correctly fired in case of invariant load hoisting being disabled, but I could
> > not find a good way to do so, as llvm-lit immediately aborts if a command
> > yields a non-zero return value. As we do not generally test our asserts,
> > not having a test case here seems OK.
> >
> > This resolves http://llvm.org/PR31494
> >
> > Suggested-by: Michael Kruse <llvm at meinersbur.de>
> >
> > Reviewers: efriedma, jdoerfert, Meinersbur, gareevroman, sebpop, zinob, huihuiz, pollydev
> >
> > Reviewed By: Meinersbur
> >
> > Differential Revision: https://reviews.llvm.org/D28798
> >
> > Modified:
> >      polly/trunk/lib/Analysis/ScopInfo.cpp
> >      polly/trunk/lib/CodeGen/IslNodeBuilder.cpp
> >      polly/trunk/test/Isl/CodeGen/MemAccess/invariant_base_ptr.ll
> >
> > Modified: polly/trunk/lib/Analysis/ScopInfo.cpp
> > URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopInfo.cpp?rev=292213&r1=292212&r2=292213&view=diff
> > ==============================================================================
> > --- polly/trunk/lib/Analysis/ScopInfo.cpp (original)
> > +++ polly/trunk/lib/Analysis/ScopInfo.cpp Tue Jan 17 06:00:42 2017
> > @@ -1078,16 +1078,24 @@ void MemoryAccess::setNewAccessRelation(
> >     isl_set_free(NewDomain);
> >     isl_set_free(StmtDomain);
> >   
> > -  // Check whether access dimensions correspond to number of dimensions of the
> > -  // accesses array.
> >     auto *NewAccessSpace = isl_space_range(NewSpace);
> >     assert(isl_space_has_tuple_id(NewAccessSpace, isl_dim_set) &&
> >            "Must specify the array that is accessed");
> >     auto *NewArrayId = isl_space_get_tuple_id(NewAccessSpace, isl_dim_set);
> >     auto *SAI = static_cast<ScopArrayInfo *>(isl_id_get_user(NewArrayId));
> >     assert(SAI && "Must set a ScopArrayInfo");
> > -  assert(!SAI->getBasePtrOriginSAI() &&
> > -         "Indirect array not supported by codegen");
> > +
> > +  if (SAI->isArrayKind() && SAI->getBasePtrOriginSAI()) {
> > +    InvariantEquivClassTy *EqClass =
> > +        getStatement()->getParent()->lookupInvariantEquivClass(
> > +            SAI->getBasePtr());
> > +    assert(EqClass &&
> > +           "Access functions to indirect arrays must have an invariant and "
> > +           "hoisted base pointer");
> > +  }
> > +
> 
> This is going to produce an "unused variable EqClass" warning in builds 
> with assertions turned off; maybe put the whole if statement into an 
> "#ifndef NDEBUG"?

Thank you Eli for this comment. In fact, this code is already guarded by
an NDEBUG section. However, as we do quite some sanity checking here the
section is too large to be visible in the diff.

Best,
Tobias

> 
> -Eli
> 
> -- 
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits