[PATCH] D16878: [POLLY] Support accesses with differently sized types to the same array

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

thanks for the quick review. I replied inline.

Regarding the alignment test case. I renamed it and made clear that this is not about alignment, but about the offset expression being a multiple of the element size. A restriction, we have today and which is left untouched by this patch.

Best,
Tobias


================
Comment at: include/polly/Support/ScopHelper.h:151
@@ -141,1 +150,3 @@
+  }
+
   llvm::Value *getValueOperand() const {
----------------
jdoerfert wrote:
> Can we use this somewhere else too? If so we might want to do so and commit it ahead of time.
I just went through the code, but did not find an obvious example. I could add it e.g. in the invariant load hoisting, but it would just add an unnecessary layer of indirection. So I think we can not move this out of this patch.

================
Comment at: lib/Analysis/ScopInfo.cpp:205
@@ +204,3 @@
+  return ValA;
+}
+
----------------
jdoerfert wrote:
> Maybe you can use:
> 
> ```
>   APInt GreatestCommonDivisor (const APInt &Val1, const APInt &Val2)
> ```
> 
> or 
> 
> ```
>   std::experimental::gcd
> ```
> 
> from here http://en.cppreference.com/w/cpp/experimental/gcd .
> 
I now use GreatestCommonDivisor64 from include/llvm/Support/MathExtras.h

================
Comment at: lib/Analysis/ScopInfo.cpp:496
@@ +495,3 @@
+  return isl_map_lexmin(getAccessRelation());
+}
+
----------------
jdoerfert wrote:
> Shouldn't the structute allow us to look for the lower bound instead of the lexmin?
What do you mean by "lower bound". Is there another isl function I could use instead of lexmin? (for me lexmin is the smallest value, so it seems to be some kind of lowerbound).

================
Comment at: test/ScopInfo/multiple-types-non-power-of-two.ll:24
@@ +23,3 @@
+; not rounded up to the next power-of-two allocation size, but rather to the
+; next multiple of 64.
+
----------------
jdoerfert wrote:
> I do not get the part about the allocation sizes. Where do you verify them in this test case?
I added the following text:

```
; The allocation size discussed above defines the number of canonical array      
; elements accessed. For example, even though i27 only consists of 3 bytes,      
; its allocation size is 4 bytes. Consequently, we model the access to an        
; i27 element as an access to four canonical elements resulting in access        
; relation constraints '4i0 <= o0 <= 3 + 4i0' instead of '3i0 <= o0 <= 2 + 3i0'.
```

Did this make the test case more clear?

================
Comment at: test/ScopInfo/multiple-types-unaligned.ll:5
@@ +4,3 @@
+;    // The following statements are not aligned to a multiple of the
+;    // type size.
+;    void multiple_types(char *Short, char *Float, char *Double) {
----------------
jdoerfert wrote:
> You don't know that. Same as you don't know that they are in the example below. As long as there is no alignment information attached to the pointers it is not clear how the alignment is and how it has to be in order to access the memory through this pointers. Not all machines require aligned accesses...
For LLVM some alignment information is always known, it is either given explicitly with an alignment annotation or implicitly through the target data interface. For most architectures this means that types are  by-default aligned to a multiple of their type size. So yes, we always get some information about alignment guarantees.

However, the term alignment is a little confusing here and the above alignment is not what this is about. We use the term unaligned in Polly to talk about access functions for which the offset from the base pointer can not be evenly divided by the type size, as we otherwise can not generate an access function for it (without falling back to smaller types). This restriction has been introduced by you as a correctness fix in https://llvm.org/svn/llvm-project/polly/trunk@252942.

Regarding this patch: we just leave the existing restriction in place (and we just add a test to verify that this is indeed the case). As written in the comment, this could be allowed as a straightforward extension. However, to keep this patch small, I focused on what I see as the more common case and left this for a later extension. Even so this is surely a nice extension, I do not see an inherent need why this needs to be part of an initial implementation.

I could add a brief explanation to this test case to make this difference clear.


http://reviews.llvm.org/D16878