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

[Tobias Grosser via llvm-commits]

grosser added a comment.

Last comments inline.


================
Comment at: include/polly/Support/ScopHelper.h:151
@@ -141,3 +150,3 @@
   llvm::Value *getValueOperand() const {
     if (isLoad())
       return asLoad();
----------------
Meinersbur wrote:
> jdoerfert wrote:
> > This method (as well as most of the ones above) is now just a plain wrapper around the same one in llvm::Instruction. I argued this before and will do it again: __ Do not copy all these functions but instead use the original __
> > 
> > 
> > ```
> > MAcc.asInstruction()->getDebugLoc();
> > ```
> > 
> > is not soo bad is it?
> The model CallSite class also has some of these forwarders. In my original design, MemAccInst was derived from llvm::Instruction st this was no issue.
> 
> Suggestion:
> ```
> Instruction *operator->() const { return I; }
> ```
> and use like this:
> ```
> MAcc->getDebugLoc();
> ``` 
I still like the shorter version.

Adding new, trivial functions out-of-line seems to be minimal cost whereas adding (even so minimal) complexity in already non-trivial functions is something I am slightly worried about.

Clearly this is nothing super important, so if you feel strong about this I could change this.

================
Comment at: test/ScopInfo/multiple-types-non-power-of-two.ll:26
@@ +25,3 @@
+
+; The allocation size discussed above defines the number of canonical array
+; elements accessed. For example, even though i27 only consists of 3 bytes,
----------------
Sorry, i27 was a mistake. It should be i24. Then the explanation makes sense.

Regarding the allocation size. We derived it using DL.getTypeAllocSize(), so it was already target specific. I now use the function getTypeAllocSizeInBits(), which allows us to also get rid of the magic constant '8'.


http://reviews.llvm.org/D16878