[polly] r259587 - Support loads with differently sized types from a single array

[Tobias Grosser via llvm-commits]

Hi Johannes,

thank you for the feedback.

On 02/02/2016 11:39 PM, Johannes Doerfert wrote:
> While I think we should allow accesses (not only loads) to the same base
> pointer with different sizes (I mentioned this multiple times before), I
> do not understand your patch. It does some lexmin magic and the comments
> seem to be broken. In general I was wondering why you assume
>    polly/trunk/test/ScopInfo/multiple-types-incorrect.ll
> to be "incorrect" (without reverting to C semantics).

OK. It seems the patch was not sufficiently clear and needs more 
discussion. I reverted it in 
https://llvm.org/svn/llvm-project/polly/trunk@259629 to allow us to do a 
full code-review.

>
> This said I would like three things from you now:
>    1) Please explain this patch and fix the first two comments I
>       inlined.

A better documented version of this patch is available at:

	http://reviews.llvm.org/D16878

Let me know (in the review), if parts remain difficult to understand.

>    2) Explain why this approach is right/better than the one I proposed
>       at some point (see attachment).

I remember discussing the need to add this kind of type handling, but I 
unfortunately never saw this patch/proposal you just sent out. Did I 
miss a phabricator / email? Could you point me to this one?

I just looked at your patch. Unfortunately it does not apply so I can 
not test it easily and - without test cases in your patch - I am also
not sure which cases it can handle. I tried to rebase it quickly, but 
then run into an assert for several of my test cases.

Just from looking at the source code, these issues seem to not be handled:

1) Code generation
2) Using the gcd for types that are not multiples of each other
3) Bailing out for multi-dimensional parametric arrays

I would be glad to incorporate some improvements/simplifications from 
your patch. Feel invited to suggest them in the patch review.

>    3) Think about pushing such patches on the review server first. While
>       I know from experience that it can take quite a while to get
>       feedback there, it seems odd when changes like this are just pushed
>       without any kind of discussion or code review. If I am wrong about
>       the last part, I will gladly revisit my policy of pushing patches
>       to the review server first.

Sure. The patch is available on phabricator now.

Best,
Tobias