[PATCH] D33688: [Polly] Heap allocation for new arrays

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1419
+      // Get the size of the element type in bits
+      unsigned Size = SAI->getElementType()->getPrimitiveSizeInBits() / 8;
+
----------------
There is `ScopArrayInfo::getElemSizeInBytes()` which should be the standard way to get an element's size, unless you have a reason why you need something different.


================
Comment at: lib/Exchange/JSONExporter.cpp:709
+      if (Size <= 0) {
+        errs() << "The size at index " << i << " is =< 0.\n";
+        return false;
----------------
The additional range check could be committed separately, it look unrelated.
(Andreas can just commit it with its test case)


================
Comment at: lib/Exchange/JSONExporter.cpp:722
+        auto CurrentSAI =
+            S.getArrayInfoByName(Arrays[ArrayIdx]["name"].asCString());
+        CurrentSAI->setIsOnHeap(true);
----------------
simbuerg wrote:
> niosega wrote:
> > simbuerg wrote:
> > > niosega wrote:
> > > > I need to have a pointer to the newly created ScopArrayInfo to call the setter.
> > > > But the method createScopArrayInfo returns only a const SAI *. The solution 
> > > > I found is to query the Scop to obtain the SAI by name.
> > > > 
> > > > An alternative would be to pass a parameter to createScopArrayInfo then to 
> > > > getOrCreateScopArrayInfo that represents the value that isOnHeap must take.
> > > This is just to circumvent an inconvenient API that you want to add with this patch. I would suggest a simpler way:
> > > Instead of depending on the setter, just add the IsOnHeap property as a function argument to
> > > createScopArrayInfo and pass it through to getOrCreateScopArrayInfo. There you can pass it to the constructor or use
> > > your setter.
> > To pass the parameter through createScopArrayInfo then getOrCreateScopArrayInfo  then to the constructor is what I did in the previous version of the patch. Michael and you agreed that it was not the prettiest solution. Should I change it back ?
> Well the pass-through is ugly too :-).
> So far, I see 3 options:
> 
> 1) Pass it through everything.
> 2) Remove the const-ness of createScopArrayInfo
> 3) Make IsOnHeap mutable.
> 
> I just dislike the lookup by name to get a non-const ScopArrayInfo. Michael what do you think?
> If nobody is objecting the name-lookup, I'm fine with it as well.
I prefer removing the `const` which IMHO serves no purpose. Some of its methods like `updateElementType` already do modify the SAI, and it should not matter how one gets the reference to it.

`Scop::updateAccessDimensionality()` already does an `const_cast` in order to be able to call `updateElementType`.


https://reviews.llvm.org/D33688