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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a subscriber: gareevroman.
Meinersbur added inline comments.


================
Comment at: include/polly/ScopInfo.h:359
 
+  /// Is this array allocated on heap
+  bool isOnHeap() const { return IsOnHeap; }
----------------
Please add the information that the property is only relevant if the array is allocated by Polly instead of pre-existing. Also, that when it is false, it is allocated using `alloca` (instead of `malloca`)


================
Comment at: include/polly/ScopInfo.h:419
 
+  /// True if the newly allocated array is on heap. False otherwise.
+  bool IsOnHeap;
----------------
... is allocated on the heap. The "False otherwise" does not add any information, it's a boolean.


================
Comment at: include/polly/ScopInfo.h:2590
+      Value *BasePtr, Type *ElementType, ArrayRef<const SCEV *> Sizes,
+      MemoryKind Kind, const char *BaseName = nullptr, bool IsOnHeap = false);
 
----------------
Please document the new parameter.

Instead of passing to every constructor, you could also defeault-initialize it to false and add a `setIsOnHeap` accessor (or similar name) to change that property after creation.


================
Comment at: include/polly/ScopInfo.h:2600
+                                           const std::vector<unsigned> &Sizes,
+                                           bool IsOnHeap = false);
 
----------------
Please document the new parameter.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1430-1434
+      auto *CreatedArray = CallInst::CreateMalloc(
+          &*InstIt, IntPtrTy, SAI->getElementType(),
+          ConstantInt::get(Type::getInt64Ty(Ctx), Size),
+          ConstantInt::get(Type::getInt64Ty(Ctx), ArraySizeInt), nullptr,
+          SAI->getName());
----------------
simbuerg wrote:
> Meinersbur wrote:
> > Where is the memory free'd?
> Good Catch. We just discussed possible locations for the free. The easiest way would be the exit(s) of the SCoP. However, to keep it simple, we would have to do a copy-in/-out to the original array base pointer, right? Everything else (e.g. calculating lexicographic maximal accesses) would give us trouble with non-polyhedral accesses between SCoPs.
copy-in/out is not specific to heap arrays. Such things can be done using with additional copy statements.

For scalar expansion, the scalar are not available after the SCoP anyway, with the exception of 'escaping' scalars. In that case only a single value is available to the outside. I suggest to exclude escaping scalars at the moment.

Here, alloca/malloc are generated when entering the Scop. I think free'ing it when exiting it is the most obvious choice.

@gareevroman
Btw, for alloca this location looks to be the wrong choice. alloca's should be in a function's entry block. The SCoP itself could be within the loop, meaning that everytime the SCoP is executed, the stack grows, up to a possible stack overflow.


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1430-1434
+      auto *CreatedArray = CallInst::CreateMalloc(
+          &*InstIt, IntPtrTy, SAI->getElementType(),
+          ConstantInt::get(Type::getInt64Ty(Ctx), Size),
+          ConstantInt::get(Type::getInt64Ty(Ctx), ArraySizeInt), nullptr,
+          SAI->getName());
----------------
philip.pfaffe wrote:
> simbuerg wrote:
> > Meinersbur wrote:
> > > Where is the memory free'd?
> > Good Catch. We just discussed possible locations for the free. The easiest way would be the exit(s) of the SCoP. However, to keep it simple, we would have to do a copy-in/-out to the original array base pointer, right? Everything else (e.g. calculating lexicographic maximal accesses) would give us trouble with non-polyhedral accesses between SCoPs.
> Couldn't that create use-after-free if the SCoP is within a loop?
@gareevroman
I was wrong, the alloca is actually added to the entry block.

This means that also the `malloc` is in the entry block, but the call to `free` below is added into last block of the //orginal// region (where it is not even used). We either have to

1. Add the malloc to the entry block and the free at every `ret` (or non-returning function call)

- or -

2. Add the malloc to the start of the generated code (`polly.start`) and the free at the end of it (`polly.exiting`).


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1398
+    // Get the size of the array = size(dim_1)*...*size(dim_n)
+    unsigned long int ArraySizeInt = 1;
     for (int i = SAI->getNumberOfDimensions() - 1; i >= 0; i--) {
----------------
On 32-bit platforms and 64-but windows, `long` has only 32 bits. You test case failsdue to an overflow:
```
polly\test\Isl\CodeGen\MemAccess\create_arrays_heap.ll:35:12: error: expected string not found in input
; CODEGEN: %malloccall1 = tail call i8* @malloc(i64 432537600000)
           ^
<stdin>:12:2: note: scanning from here
 %malloccall1 = tail call i8* @malloc(i64 20220739584)
 ^

error: command failed with exit status: 1
```


================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1433
+      // Insert the free call at the end of the scop
+      auto *FreedArray = CallInst::CreateFree(
+          SAI->getBasePtr(), S.getExitingBlock()->getTerminator());
----------------
The variable `FreedArray` isn't used anywhere.


================
Comment at: lib/Exchange/JSONExporter.cpp:710
+      StringRef AllocationString(Arrays[ArrayIdx]["allocation"].asCString());
+      StringRef HeapString("heap");
+      if (AllocationString.compare(HeapString) == 0)
----------------
There is no necessity to store temporary `StringRef` for a literal string. `AllocationString == "head"` should just work.


================
Comment at: test/Isl/CodeGen/MemAccess/create_arrays_heap.ll:1
+; RUN: opt %loadPolly -mem2reg -polly-scops -analyze -polly-import-jscop-dir=%S -polly-import-jscop -polly-import-jscop-postfix=transformed < %s 2>&1 | FileCheck %s
+; RUN: opt %loadPolly -mem2reg -polly-import-jscop-dir=%S -polly-import-jscop -polly-import-jscop-postfix=transformed -polly-codegen -S < %s 2>&1 | FileCheck %s --check-prefix=CODEGEN
----------------
`mem2reg` does not need to be part of a test. You can invoke `opt create_arrays_heap.ll -mem2reg -S` and use that output for the test case.

Does this test require `2>&1`?


https://reviews.llvm.org/D33688