Relax assert when setting access functions with invariant base pointers

[grosser]

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.

Suggested-by: Michael Kruse <[e-mail]>

[Meinersbur]

lib/Analysis/ScopInfo.cpp

1081–1082

This comment now seems detached from the asset in line 1101 due to all the empty lines inserted.

1088

Unrelated new line

1095

When assertions are off, the compiler will warn about EqClass being unused. There are are function calls which are not going to be optimized away.

Did you consider to put #ifndef NDEBUG around this?

[grosser]

Thank you for the fast review. I addressed your last comments and committed.

lib/Analysis/ScopInfo.cpp

1081–1082

I will move this down.

1088

I will drop this.

1095

This is a NDEBUG region (which you introduced). It is just rather large and so not easily visible in the patch.

lib/Analysis/ScopInfo.cpp

#ifndef NDEBUG
   StmtDomain = isl_set_intersect_params(
       StmtDomain, getStatement()->getParent()->getContext());
   auto *NewDomain = isl_map_domain(isl_map_copy(NewAccess));
   assert(isl_set_is_subset(StmtDomain, NewDomain) &&
          "Partial accesses not supported");
   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");
+  }
+
   auto Dims = SAI->getNumberOfDimensions();
   assert(isl_space_dim(NewAccessSpace, isl_dim_set) == Dims &&
          "Access dims must match array dims");
   isl_space_free(NewAccessSpace);
   isl_id_free(NewArrayId);
 #endif
 
   isl_map_free(NewAccessRelation);

lib/CodeGen/IslNodeBuilder.cpp

if (!MA->hasNewAccessRelation()) {
         if (BasePtr && Stmt->getParent()->getRegion().contains(BasePtr))
           continue;
       } else {
         continue;
       }
     }
     assert(MA->isAffine() &&
            "Only affine memory accesses can be code generated");
-    assert(!MA->getLatestScopArrayInfo()->getBasePtrOriginSAI() &&
-           "Generating new index expressions to indirect arrays not working");
 
     auto Schedule = isl_ast_build_get_schedule(Build);
 
 #ifndef NDEBUG
     auto Dom = Stmt->getDomain();
     auto SchedDom = isl_set_from_union_set(
         isl_union_map_domain(isl_union_map_copy(Schedule)));
     auto AccDom = isl_map_domain(MA->getAccessRelation());

test/Isl/CodeGen/MemAccess/invariant_base_ptr.ll

 ; RUN: opt %loadPolly -polly-import-jscop -polly-import-jscop-dir=%S \
-; RUN:   -polly-codegen -analyze 2 >&1 < %s | FileCheck %s
-
-; XFAIL: *
+; RUN:   -polly-codegen -polly-invariant-load-hoisting -S \
+; RUN:   2>&1 < %s | FileCheck %s
 
 ; Setting new access functions where the base pointer of the array that is newly
 ; accessed is only loaded within the scop itself caused incorrect code to be
-; generated when invariant load hoisting is disabled. Since r282893 we assert
-; in such situations. This test case was added to demonstrate what needs to be
-; resolved to support such access functions.
+; generated when invariant load hoisting is disabled. This test case checks
+; that in case invariant load hoisting is enabled, we generate correct code.
 
 ; CHECK: %polly.access.polly.access.X.load = getelementptr float, float* %polly.access.X.load, i64 %polly.indvar
 
 define void @invariant_base_ptr(float* noalias %Array, float** noalias %X,
                                 float* noalias %C) {
 
 start:
   br label %loop