This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Introduce the ScopArrayInfo class.
ClosedPublic

Authored by jdoerfert on Oct 3 2014, 6:45 PM.

Details

Summary
This class allows to store information about the arrays in the SCoP.
For each base pointer in the SCoP one object is created storing the
type and dimension sizes of the array. The objects can be obtained via
the SCoP, a MemoryAccess or the isl_id associated with the output
dimension of a MemoryAccess (the description of what is accessed).

So far we use the information in the IslExprBuilder to create the
right base type before indexing into the base array. This fixes the
bug http://llvm.org/bugs/show_bug.cgi?id=21113 (both test cases are
included). On top of that we can now build runtime alias checks for
delinearized arrays as the dimension sizes are also part of the
ScopArrayInfo objects.

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert updated this revision to Diff 14419.Oct 3 2014, 6:45 PM
jdoerfert retitled this revision from to Introduce the ScopArrayInfo class..
jdoerfert updated this object.
jdoerfert edited the test plan for this revision. (Show Details)
jdoerfert added subscribers: Restricted Project, Unknown Object (MLST).
jdoerfert retitled this revision from Introduce the ScopArrayInfo class. to [Polly] Introduce the ScopArrayInfo class..Oct 3 2014, 6:45 PM
grosser edited edge metadata.Oct 3 2014, 10:14 PM

Very nice. Besides fixing two important bugs in the run-time alias checks, this is also conceptually a big step forward. Thanks for working on this last night!

The patch looks great, with just some test-case changes that I unfortunately do not understand.

I also added a couple (3-4) of minor comments on your code. None of them has correctness implications and the original code is already very nice, so just include them as you like.

include/polly/CodeGen/IslExprBuilder.h
84 ↗(On Diff #14419)

It is unclear to me why the IslExpr Builder would need a SCoP now.

lib/Analysis/ScopInfo.cpp
294 ↗(On Diff #14419)

A similar function that takes an isl_id and returns the ScopArrayInfo might be handy too. (See later comment on how to obtain the BasePtr/ElementType of an isl_ast_op_access).

475 ↗(On Diff #14419)

Conceptually it probably makes sense to allocate one isl_id in the ScopArrayInfo class and then give users the option to obtain copies of this id. This makes clear that all these isl_ids are identical. (The current code also works at the moment AFAIK, as isl assumes two ids are identical if their name and the user pointer are identical).

lib/CodeGen/IslCodeGeneration.cpp
58 ↗(On Diff #14419)

I am surprised we need the Scop here.

lib/CodeGen/IslExprBuilder.cpp
113 ↗(On Diff #14419)

Instead of passing the scop to this place and use the base ptr to look up the ScopArrayInfo, you could possibly also obtain the isl_id from the isl_ast_expr_get_op_arg(Expr, 0) and then use a "static ScopArrayInfo::getFromid(Id)" function.

Also, instead of obtaining the Base pointer by calling create(), we could obtain
it directly from this ScopArrayInfo object. This would allow us to remove
IslNodeBuilder::addMemoryAccesses(), which was mostly added due to us missing a ScopArrayInfo class.

137 ↗(On Diff #14419)

Nice cleanup!

test/Isl/CodeGen/MemAccess/codegen_simple.ll
43 ↗(On Diff #14419)

It is unclear to me why this test case needs to be modified and especially why the generated IR now can be have one of two forms. Is the IR we generate not supposed to be predictable?

Responses. I would try to change the code according to the comments but I still like the changes to the tests ({{(i32}i64)}} instread of i32)

include/polly/CodeGen/IslExprBuilder.h
84 ↗(On Diff #14419)

To access the ScopInfoArray for a base pointer.

lib/Analysis/ScopInfo.cpp
475 ↗(On Diff #14419)

I'll change that. Good point.

lib/CodeGen/IslCodeGeneration.cpp
58 ↗(On Diff #14419)

Just to pass it on to the IslExprBuilder.

lib/CodeGen/IslExprBuilder.cpp
113 ↗(On Diff #14419)

True, I'll try that and keep it if it works

test/Isl/CodeGen/MemAccess/codegen_simple.ll
43 ↗(On Diff #14419)

It has a different form now because for A[100]

CreateBitcast(A[0]) == yields ==> getelemenptr inbounds [100 x i32]* @A, i32 0, i32 0

where the indices are of type i32.

Before the patch we added the indices and used the typ of the only real index op to create "zeros" (thus they were i64). Now I don't want to hardcode i32 because a change in the bitcast function or in our code could revert this or mix the indice types without a semantical change, thus I don't see the reason to fix one type here.

jdoerfert updated this revision to Diff 14426.Oct 4 2014, 9:12 AM
jdoerfert edited edge metadata.

Updated according to the comments (except test cases)

jdoerfert closed this revision.Oct 5 2014, 4:42 AM
jdoerfert updated this revision to Diff 14431.

Closed by commit rL219077 (authored by @jdoerfert).