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 ↗

(On Diff #84648)

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

1088 ↗

(On Diff #84648)

Unrelated new line

1095 ↗

(On Diff #84648)

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 ↗

(On Diff #84648)

I will move this down.

1088 ↗

(On Diff #84648)

I will drop this.

1095 ↗

(On Diff #84648)

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

polly/trunk/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");
+  }
+
+  // Check whether access dimensions correspond to number of dimensions of the
+  // accesses array.
   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);

polly/trunk/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());

polly/trunk/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