This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Runtime alias checks
ClosedPublic

Authored by jdoerfert on Aug 26 2014, 2:13 PM.

Details

Summary

Allow the IslExprBuilder to compare pointers

Ensure the runtime condition is always a i1 type

Allow the IslExprBuilder to build address of expressions

[Fix] Expression builder access expressions

Access expressions (e.g., A[i]) are now treated as left hand values.
Thus, the isl expression builder will create a load of A[i] instead of
a gep pointing to the address. If the gep is needed use the address of
operator.

Build and print alias groups

This change will build and print all alias groups (minimal/maximal
accesses to possible aliasing base pointers) we have to check before
we can assume an alias free environment.

Generate runtime alias checks.

This check will also annotate the IslAst with runtime alias checks.
Most imporantly it makes -polly-use-runtime-alias-checks=true save for
the isl code generator.

Diff Detail

Event Timeline

jdoerfert updated this revision to Diff 12968.Aug 26 2014, 2:13 PM
jdoerfert retitled this revision from to [Polly] Runtime alias checks.
jdoerfert added reviewers: grosser, sebpop, simbuerg.
jdoerfert updated this object.
jdoerfert added subscribers: Restricted Project, Unknown Object (MLST).
jdoerfert updated this revision to Diff 13410.Sep 8 2014, 11:50 AM

Fixed offset by one bug in the maximal access (now it points to the address after the last element).

grosser edited edge metadata.Sep 11 2014, 8:48 AM

Hi Johannes,

the patch looks really good for most parts.

Two remaining issues:

  • There seems to be an off by one error (< instead of <=)
  • Iteration over a SmallPtrSet may cause random changes.

A comment to think about:

  • I am unsure if we should use AST.add(LoadPtr, 8) or AST.add(BasePtr, INT_MAX)

And then a set of smaller comments on the patch.

include/polly/ScopInfo.h
505

As you iterate over this set I propose to use a SetVector. Otherwise, the iteration order may randomly change due to adress space randomization, different memory allocators or different operating systems.

549

a set of accesses.

(There is an 'a' too much)

558

typo: _an_ alias free

(Btw, the documentation above is very nice)

lib/Analysis/ScopDetection.cpp
201

We may want to get rid of those two conditions, but it seems OK to insert them for now as this can make the code review more incremental.

lib/Analysis/ScopInfo.cpp
72

This is a great feature. We should enable it by default. This also will allow our automatic testers to ensure no regressions slip in throughout time.

1151

This may cause an out of bound access in the last dimension. Was this intended and is this safe?

In case this is intended, I believe it would be good to add a comment that this case may happen and why it is safe. Nothing complicated, just to ensure people browsing the code see that we though about it.

1176

Very nice comment.

1186

This is interesting. When I was thinking of this, my thought was to use the base pointer and UINT64_MAX as size, you seem to use the pointer in the load/store instruction and the actual load/store value as size.

I am currently thinking about if there is a difference in terms of expressiveness/correctness. Honestly, I don't know yet. Did you think about the correct size? Is there a reason one approach might be incorrect?

1271

I am personally not a big fan of such somehow surprising line saving tricks.

1578

As mentioned earlier, it would be nice to only call this on-demand in getAliasGroups(), as users who only need to analysis part of Polly should not need to pay the cost of building alias groups. The same holds in case we want to bail out from optimizing a scop.

We have specific performance testers that ensure that -polly-scops is not getting more expensive over time. Having them not note this change would be ensuring that we did not increase compile time in the early phase of Polly.

I remember you felt very strong about not moving this piece of code over. If you still believe that way, I won't discuss this further.

lib/CodeGen/IslAst.cpp
302

This change is the reason why we suddenly get conditions such as:

if (1 == 1)

It is not of cricital importance, but you could probably get rid of them by applying the following patch to createOpBoolean:

-  assert(LHS->getType() == Builder.getInt1Ty() && "Expected i1 type");
-  assert(RHS->getType() == Builder.getInt1Ty() && "Expected i1 type"); 
+  if (LHS->getType() != Builder.getInt1Ty()) 
+    LHS = Builder.CreateIsNotNull(LHS);
+  if (RHS->getType() != Builder.getInt1Ty()) 
+    RHS = Builder.CreateIsNotNull(RHS);

This would also reduce the number of test cases that get touched by this commit, which again helps people doing post-commit review or who later look at the patch.

305

typo: group

lib/CodeGen/IslCodeGeneration.cpp
596

Not needed if you auto booleanize values in createOpBoolean()

test/Isl/Ast/alias_simple_1.ll
16

I think this should be '<=' instead of '<'.

Assuming this is:

0 1 2 3 4 5 6 7
x x x x x x x x

^A ^B

// N = 2

short A[N]
short B[N]

&A[N] => Base + (2 * sizeof(short)) = 0 + 2 * 2 = 4
&B[0] => Base + (0 * sizeof(short)) = 4 + 0 * 2 = 4

&A[N] < &B[N] is false, but indeed it should evaluate to true

test/Isl/Ast/run-time-condition.ll
31 ↗(On Diff #13546)

Not needed if you auto booleanize values in createOpBoolean()

test/Isl/Ast/simple-run-time-condition.ll
21 ↗(On Diff #13546)

Not needed if you auto booleanize values in createOpBoolean()

test/Isl/CodeGen/multidim_2d_parametric_array_static_loop_bounds.ll
16 ↗(On Diff #13546)

Not needed if you auto booleanize values in createOpBoolean()

test/Isl/CodeGen/run-time-condition-with-scev-parameters.ll
7 ↗(On Diff #13546)

Not needed if you auto booleanize values in createOpBoolean()

test/Isl/CodeGen/scop_never_executed_runtime_check_location.ll
14 ↗(On Diff #13546)

Not needed if you auto booleanize values in createOpBoolean()

test/ScopDetectionDiagnostics/ReportAlias-01.ll
9

For me, the test case still produces "A", "B" as output. Is there a specific reason this needs to be changed (e.g. your system gives "B", "A")?

I am asking as unpredictable output sometimes points to cases where we loop over unsorted sets, which then cause this kind of behaviour that is hard to debug and consequently better avoided.

I did not mention this before, but the test cases look all very nice. It is good to have proper test coverage of this feature.

Comments

include/polly/ScopInfo.h
505

Ok.

549

Fixed.

558

Fixed. Thanks.

lib/Analysis/ScopDetection.cpp
201

In the long run we want to remove them but we cannot turn RuntimeAliasChecks on by default without them atm.

lib/Analysis/ScopInfo.cpp
72

Enabled by default in the last commit of this series.

1151

Added a sentence, just stating the possibility and that it is safe.

1176

thx.

1186

I'm sorry but I don't understand what you are saying. I also do not know why we should use UINT64_MAX except when we can't represent the real offset. However, the runtime alias checks are disabled for now in case "allow non affine" is enabled, thus we always know the exact (parametric) offset.

Could you explain your idea and the problem this approach might have in a bit more detail?

1271

Changed it.

1578

Your right, I still like the code here, but I also gave you some reasons for this.

One more (wrt. what you said about polly as analyser) is that we can provide exact information about possibly overlapping accesses. I will use this feature together with the dependency analysis in another project, both with and without generating an optimized SCoP with Polly.

lib/CodeGen/IslAst.cpp
302

Changed + removed the patch "force rtc to be i1 type".

305

fixed.

lib/CodeGen/IslCodeGeneration.cpp
596

I changed the assert to an if + createIsNotNull.

test/Isl/Ast/alias_simple_1.ll
16

If I get your example correctly we have:

short *A = ...
short *B = A + 2;

and accesses (with N=2):

A[2] and B[0]

then the alias check should say false (what it does) becasue both accesses will load the same value.

What do you think?

test/Isl/CodeGen/multidim_2d_parametric_array_static_loop_bounds.ll
16 ↗(On Diff #13546)

As you see on the left, either a "!=" or "==" comparison is needed.

test/Isl/CodeGen/run-time-condition-with-scev-parameters.ll
7 ↗(On Diff #13546)

As you see on the left, either a "!=" or "==" comparison is needed.

test/Isl/CodeGen/scop_never_executed_runtime_check_location.ll
14 ↗(On Diff #13546)

As you see on the left, either a "!=" or "==" comparison is needed.

test/ScopDetectionDiagnostics/ReportAlias-01.ll
9

It doesn't now but it did say "B", "A" for a while.

I changed it back to only match "A", "B" but someone might want to take a look at this.

Hi Johannes,

here some feedback to your comments.

Tobias

P.S: Did you push the changed patch? I can not find it in the web interface.

lib/Analysis/ScopInfo.cpp
1151

I think it would be helpful to mention why we believe it is safe. Otherwise no-one can verify if our assumptions still hold e.g., after changes to LLVM.

1186

In the ScopDetection we use the following code to check for aliasing:

// Check if the base pointer of the memory access does alias with              
// any other pointer. This cannot be handled at the moment.                    
AliasSet &AS =                                                                 
  Context.AST.getAliasSetForPointer(BaseValue, AliasAnalysis::UnknownSize,   
                                        Inst.getMetadata(LLVMContext::MD_tbaa));

Similarly, the AST has a set of add() functions:

bool add(Value *Ptr, uint64_t Size, const AAMDNodes &AAInfo); // Add a loc.    
bool add(LoadInst *LI);                                                        
bool add(StoreInst *SI);

The LoadInst/StoreInst version of the add reason about a single load of maybe 16 bytes, whereas the first version of the add instruction allows to give a base pointer and an arbitrary offset.

The reason I implemented the alias check in the scop detection using the base pointer + an UnknownSize parameter is that I wanted to include all possible (positive?) offsets from this base pointer in the alias check.

This is similar to how we in the end construct the run-time alias check. Only checking the individual loads did not come to my mind.

Now, as I said, I don't really know what is better/more correct.

Some first thoughts are here:

When checking individual accesses I could imagine that a single base array is split into multiple alias sets:

for: int i
  S1:  A[2 i] = A[2i + 1]; 
  S2:  A[2i + 2] = A[2i - 1];

Here an alias analysis could possibly form two groups AS_1 = {A[2i], A[2i + 2]}, AS_2 = {A[2i+1], A[2i-1]},
as the even and odd accesses could be proven to not alias. Would this have any drawbacks?

When checking the baseptr + large-offset, we may have troubles to exploit the tbaa and other per-access annotations. Is this right?

In case we decide to go for the per-access checks, I wonder if we then should change the check in the scop detection as well?

1578

Just to make it clear. I agree that all the code should remain in ScopInfo. The only change I suggested was to not call buildAliasGroups() in runOnScop, but rather on-demand from getAliasGroups() (and to then use getAliasGroups() in the analysis-printer).

test/Isl/Ast/alias_simple_1.ll
16

My example extends the code in the test case by defining a specific N and a specific memory address for A and B. For this N, the test case only accesses A[0], A[1], B[0], B[1], but not A[2]. For the pointer offsets specified (short *B = A+2), this means there are no conflicting accesses. However, as you seem to agree the alias check will detect the existence of aliasing. To me this seems overly and unnecessarily conservative. Especially as it is not unlikely that two arrays are allocated right after each other.

test/Isl/CodeGen/multidim_2d_parametric_array_static_loop_bounds.ll
16 ↗(On Diff #13546)

To clarify: The instruction is needed, but most likely the change from "!=" to "==" disappears when using CreateIsNotNull(). This reduces the set of test cases that is changed, which is by itself already a good thing.

Comments, new version is comming up.

lib/Analysis/ScopInfo.cpp
1186

The ScopDetection implementation shouldn't matter as long as runtime alias checks are turned on.
Changing it to the way I implemented it here is a different matter for another time.

I now understood your problem partially, however I still do not know why I shouldn't use the

AST.add(Instruction *)

interface which then will sue the LoadInst and StoreInst versions. AST offers this, thus it should do the correct thing with the pointer operand. And since I add all memory instructions it also knows about all pointer operands.

The single base address problem is handled here anyway and the check is cheap.

I'm not a hundred procent sure about the TBAA information but I suspect your right. They are attached to the load/store instructions and we need the AST to see them.

I vote very strongly for the load/store version over the baseptr + unknown offset version here, in ScopDetection I do not care to much as the code is alsmost dead after this commit anyway.

1578

You can change that afterwards, we would also see how much compile time it safes us that way. I'm at the moment not sure e.g., how to mark the alias checks as computed or not.

test/Isl/Ast/alias_simple_1.ll
16

I think your right. I'll change that.

jdoerfert updated this revision to Diff 13630.Sep 12 2014, 6:55 AM
jdoerfert edited edge metadata.

Some fixes according to the review

grosser accepted this revision.Sep 19 2014, 1:44 AM
grosser edited edge metadata.

This code is good and was committed in r218046.

This revision is now accepted and ready to land.Sep 19 2014, 1:44 AM
grosser closed this revision.Sep 19 2014, 1:45 AM