Hi Michael,

According to the r282893 [1], we could probably trigger that the base pointer has been already generated. Could you please advise me how to do it?

Refs.:

[1] - http://llvm.org/viewvc/llvm-project?view=revision&revision=282893

Hi Roman,

sorry for the delay, I wasn't very active during the holidays.

Tobias seems to already have partially tackled this in https://reviews.llvm.org/D28518. However, the code generation part might not work with this, without actually having tried.

If we try to reuse the old pointer, as you suggested, BlockGenerator::getNewValue() needs to handle this. The baseptr value could be in BBMap due to having been hoisted.

If handling it as a NewAccessRelation like D28518 does, the "If (AccessExpr) of generateLocationAccessed()" would handle it. It implicitly would also use the hoisted baseptr from BBMap. Note that this case can always be triggered if setNewAccessRelation is used by e.g. JSONImporter.

If these indeed already work, the only thing to do in addition to D28518 is to exceptionally allow load-hoisted baseptrs in the assertion. Eg.

BasePtr = MA->getLatestScopArrayInfo()->getBasePtrOriginSAI();
assert((!BasePtr || isLoadHoisted(BasePtr)) && "Generating new index expressions to indirect arrays not working");

with some implementation of isLoadHoisted(). SCoP::lookupInvariantEquivClass could be useful.

I yesterday discussed this issue with Michael. One observation I made is that in Polly the only base pointers we allow are always loop invariant, hence it seems that the cases that Michael listed always hold and this assert is not really needed.

However, Michael remembered that he committed this assert because of a bug he was seeing in his work. Unfortunately, the corresponding test case was not committed.

It seems we should understand if this assert is really needed. Michael was trying to see if he can remember / recover the failing test case.

When I add assertions it is usually because it occurred during a bug or happened while I was trying out something. There is no bug associated with it, which means I induced it with experimental code, found it non-working, and added the assertion to make life easier in case it happens again. In this case I was not aware that it was supposed to be working if the load-hoisted base pointers. There is no test case that failed nor anything in the test-suite.

Looking at the code in ScopDetection which should be verifying the 'hoistibility' of the base pointer in ScopDetection::isValidAccess, it seems incomplete.

if (!AS.isMustAlias()) {
if (PollyUseRuntimeAliasChecks) {
[...]
for (const auto &Ptr : AS) {
Instruction *Inst = dyn_cast(Ptr.getValue());
if (Inst && Context.CurRegion.contains(Inst)) {
auto *Load = dyn_cast(Inst);
if (Load && isHoistableLoad(Load, Context.CurRegion, *LI, *SE, *DT)) {
Context.RequiredILS.insert(Load);
continue;
}

• Only enabled when AAnalysis says it might alias with something else (eg. alloca'd arrays would not alias with anything else)
• Only enabled when PollyUseRuntimeAliasChecks (on by default, but switching it off means that CodeGen starts to fail)

It might be worth to review the load-hoisting of base pointers. If it is supposed to be a working feature, could we at least add a regression test?

Thank you Michael for this information.

I do not think we ever thought about modifiable access functions for computing base pointers so it mostly works just by accident.

I agree that if we want to drop the assert again, we should review the invariant load hoisting part and address the remarks you are giving.

However, I believe that alloc-ed arrays are also checked. AFAIU will the alias analysis just generate one-element alias sets for such kind of arrays (to be verified). Assuming this is true (and you did not run with -polly-ignore-aliasing), I still do not understand in which situation this failed for you.

Not sure we an solve this, but if you have an idea that would be great.

@​Roman: If you have time it would be good to get this resolved.

It happened by calling setNewAccessRelation() with a mapping to an "indirect" array. CodeGen has a different code path for when there is a NewAccessRelation which would also have required the BasePtr to be generated.

Is it easy to generate a JSON based test case? In case it is, I could try to fix this.

Best,
Tobias

Thanks for looking into it! Sorry, I'm busy at the moment. I can try to fix it, if I have time.

Thanks for looking into it! Sorry, I'm busy at the moment. I can try to fix
it, if I have time.

I added the relevant test case in r292147.

Michael, you are right. In case we do not create alias checks we do not require the base pointers to be hoisted. That seems OK, but causes troubles when trying to rewrite such arrays. With invariant load hoisting enabled, this works reliably.

This was resolved in r292213.