[Polly][PATCH 7/8] [RTC] Build and print alias groups

[Tobias Grosser]

On 11/08/2014 10:06, Johannes Doerfert wrote:
> On 08/11, Tobias Grosser wrote:
>> >Hi Johannes,
>> >
>> >I agree with the general approach here and the patch is very nicely
>> >written.
> Thank you.
>
>> >One thing I wonder about is why you decided to build the alias checks
>> >already in ScopInfo? In your original patch they where build in the
>> >Codegeneration.cpp file, no? From my perspective this is mostly a code
>> >generation issue. So it seems building them right ahead is both conceptually
>> >a little unclean and it also means we always pay the cost of building alias
>> >checks even in cases where we do not need them.
> There are several reasons, but let me state the first and simplest one:
>    We build delinearization assumptions here too, so I create, print
>    and handle them exactly the same way, and this means:
>      1) build (and print) them in the ScopInfo pass.
>      2) transform (and pretty print) them in the IslAst pass.
>      3) generate code for them in the IslcodeGen pass.
> If you don't think this is a good approach (after reading also my next
> paragraph) i'll move it to wherever you want it to be...

The reason we build delinearization assumptions early is that we can
use them to simplify our polyhedral representation. Meaning, if we 
assume p > 0, then this can (and will) influence the dependence 
analysis. So we need this information at this point.

Is there a reason why we would need the run-time information at this point.

>> >(We do not exploit this at the moment, but I could see us stop to
>> >analyse/optimize a scop half-way through to avoid spending compile
>> >time for cases where it is obvious that there is nothing to gain.)
> Or we could check our assumptions before we go to the expansive steps
> like the dependency and scheduler pass and decide based on the number of
> aliases, the loop iterations and loop depths, and the number of memory
> accesses that we should skip the loop nest completely...
>
> As long as we do not have any kind of feeling when and where to bail for
> what kind of loop nests to save compile and runtime I don't think
> comments like this will help us developing new features. However, if you
> like me to place to code somewhere else, sure...

I believe if there is not a specific reason to compute it at another 
place, I believe we should compute information at the point where we 
need it.

>> >It may very well be that there are good reasons to build those sets early
>> >so, but you just don't mention them at all. I would have expected those sets
>> >to already be build in IslAst.cpp to be able to pretty-print them, but I am
>> >surprised they are already built in ScopInfo.cpp. Also, even if they are
>> >build later, certain pieces of the code may very well stay in ScopInfo, but
>> >may be just called later on.
> I mentioned no reasons mainly because I followed the way delinearizations
> are handled, but it seems that my code is somehow different. I will try
> to provide more justification from here on.

Not justification. Explanations of why you do something. You are the one 
who thought about it most, so it is helpful to share your thoughts.
This will make many review questions unnecessary and also helps to see 
if you did not think about certain corner cases or if you reasoning is
surprising in some way.

>>> > >  create mode 100644 test/Isl/CodeGen/MemAccess/codegen_simple_float.ll
>>> > >  create mode 100644 test/Isl/CodeGen/MemAccess/codegen_simple_md_float.ll
>>> > >  create mode 100644 test/Isl/CodeGen/MemAccess/simple___%for.cond---%for.end14.jscop
>>> > >  create mode 100644 test/Isl/CodeGen/MemAccess/simple___%for.cond---%for.end14.jscop.transformed
>> >
>> >What are you testing here? This is not clear to me.
> I accidentally added the first two files (.ll) as they are only supposed to
> extend the test coverage but not in this commit.
> The second two files (.jscop) are different as they are only a patch too early.
> Once we turn alias checks on by default we detect a larger scop in one of the
> memaccess test cases and we need those two jscop files (just a merge of
> the two existing files).
>
> I will repair that once we clarified all the issues we have with the
> whole patch set and figured out how we will proceed.

Cheers,
Tobias