This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Map the new load to the base pointer of the invariant load hoisted load
ClosedPublic

Authored by gareevroman on Mar 4 2017, 5:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grosser edited edge metadata.Mar 7 2017, 10:20 PM

Hi Roman,

as already discussed on the phone call, we certainly have a problem with the alias set annotator where no no-alias metadata is emitted for hoisted base pointers. However, it seems we do not yet fully understand the issue and the commit message and the patch itself are hard to verify. The very first step we should take is to write a minimal test case. I suggest the following one:

define void @metadata(float** %A, float** %B) {
entry:
  %ptrA = load float*, float** %A
  %ptrB = load float*, float** %B
  br label %for

for:
  %indvar = phi i64 [0, %entry], [%indvar.next, %for]
  %indvar.next = add i64 %indvar, 1
  store float 42.0, float* %ptrA
  store float 42.0, float* %ptrB
  %icmp = icmp sle i64 %indvar, 1024
  br i1 %icmp, label %for, label %exit

exit:
  ret void
}

define void @nometadata(float** %A, float** %B) {
entry:
  br label %for

for:
  %indvar = phi i64 [0, %entry], [%indvar.next, %for]
  %indvar.next = add i64 %indvar, 1
  %ptrA = load float*, float** %A
  %ptrB = load float*, float** %B
  store float 42.0, float* %ptrA
  store float 42.0, float* %ptrB
  %icmp = icmp sle i64 %indvar, 1024
  br i1 %icmp, label %for, label %exit

exit:
  ret void
}

This test case took only 10 minute to write for me. Generally it makes reviewing a lot easier if you try to get to such a minimal test case. If this does not work and you don't feel you make progress, that's OK. In this case I am happy to help. (For this patch I had the feeling you had doubts regarding how to write such a test case, which is why I went in to help you).

Looking at this test case, I believe the change itself is almost what is needed. However, plainly copying the metadata is wrong. The base pointers used in the scop should refer to "poly.alias.scope.MemRef_ptrA" and "poly.alias.scope.MemRef_ptrB", but currently refer to "poly.alias.scope.MemRef_A" and "poly.alias.scope.MemRef_B". The right approach is probably to look for the basepointer of the invariant load hoisted load and see if for this base pointer alias information is available.

Hi Tobias,

This test case took only 10 minute to write for me. Generally it makes reviewing a lot easier if you try to get to such a minimal test case. If this does not work and you don't feel you make progress, that's OK. In this case I am happy to help. (For this patch I had the feeling you had doubts regarding how to write such a test case, which is why I went in to help you).

Thanks! I think that we can also remove the array B.

Looking at this test case, I believe the change itself is almost what is needed. However, plainly copying the metadata is wrong. The base pointers used in the scop should refer to "poly.alias.scope.MemRef_ptrA" and "poly.alias.scope.MemRef_ptrB", but currently refer to "poly.alias.scope.MemRef_A" and "poly.alias.scope.MemRef_B". The right approach is probably to look for the basepointer of the invariant load hoisted load and see if for this base pointer alias information is available.

Right. Should we map the new load to the basepointer of the invariant load hoisted load to be able to do it? (I've tried to implement it in the new version of the patch).

grosser accepted this revision.Mar 12 2017, 12:49 AM

Perfect. This looks a lot better! Thanks for the cleanup!

test/Isl/CodeGen/invariant_load_alias_metadata.ll
7 ↗(On Diff #91472)

Can you include the arguments of !alias.scope and !noalias as well as the metadata referenced to the check lines?

This metadata I mean:

!0 = distinct !{!0, !1, !"polly.alias.scope.MemRef_A"}
!1 = distinct !{!1, !"polly.alias.scope.domain"}
!2 = !{!3}
!3 = distinct !{!3, !1, !"polly.alias.scope.MemRef_ptrA"}

This revision is now accepted and ready to land.Mar 12 2017, 12:49 AM
gareevroman retitled this revision from [Polly] Check for the alias set corresponding to the pointer operand's pointer operand to [Polly] Map the new load to the base pointer of the invariant load hoisted load.
gareevroman edited the summary of this revision. (Show Details)

Since, according to "assert(MA->isArrayKind() && MA->isRead())", MA models an array and it is a read memory access, AccInst, the access instruction of MA, is a base pointer and PreloadVal should be always mapped to it.

Add the removed line.

Hi Roman,

this indeed looks a lot better. Can you please include the arguments of !alias.scope and !noalias as well as the metadata referenced in the check lines, and then commit?

Best,
Tobias

This revision was automatically updated to reflect the committed changes.