[PATCH] D32961: [Polly][PPCGCodeGen] OpenCL now gets kernel argument size from PPCG CodeGen

[Siddharth via Phabricator via llvm-commits]

bollu added inline comments.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1198
                                        SetVector<Value *> SubtreeValues) {
-  Type *ArrayTy = ArrayType::get(Builder.getInt8PtrTy(),
-                                 std::distance(F->arg_begin(), F->arg_end()));
+  int NumArgs = std::distance(F->arg_begin(), F->arg_end());
+  int *ArgSizes = new int[NumArgs];
----------------
I believe `int NumArgs = F->getArgumentList().size()` will also work? seems a little clearer to me.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1198
                                        SetVector<Value *> SubtreeValues) {
-  Type *ArrayTy = ArrayType::get(Builder.getInt8PtrTy(),
-                                 std::distance(F->arg_begin(), F->arg_end()));
+  int NumArgs = std::distance(F->arg_begin(), F->arg_end());
+  int *ArgSizes = new int[NumArgs];
----------------
bollu wrote:
> I believe `int NumArgs = F->getArgumentList().size()` will also work? seems a little clearer to me.
`NumArgs` could be `const`? It doesn't seem to be mutated anywhere.


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1199
+  int NumArgs = std::distance(F->arg_begin(), F->arg_end());
+  int *ArgSizes = new int[NumArgs];
+
----------------
why not `std::vector<int>(NumArgs)`? That removes the explicit memory allocation/deallocation. I wish `std::dynarray` existed, since that's exactly the use case for this. 


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1276
+      SizeInBytes = Val->getType()->getScalarSizeInBits() / 8;
+    ArgSizes[Index] = SizeInBytes;
+
----------------
The computation of `SizeInBytes` seems to be a pure computation which is repeated thrice in this function (`GPUNodeBuilder::createLaunchParameters`). Could this be refactored into a standalone function?

That would also make the assignment look a little cleaner:
```lang=cpp
ArgSizes[Index] = computeSizeInBytes(Val->getType());
```


================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1347
+    Builder.CreateStore(ParamTyped, Slot);
+    Index++;
+  }
----------------
Could the pattern of

```lang=cpp, name=to-split.cpp
Value *Slot = Builder.CreateGEP(
        Parameters, {Builder.getInt64(0), Builder.getInt64(Index)});
    Value *ParamTyped =
        Builder.CreatePointerCast(Param, Builder.getInt8PtrTy());
    Builder.CreateStore(ParamTyped, Slot);
```

be refactored into a separate function if it is not too much trouble? It occurs thrice in this function (`GPUNodeBuilder::createLaunchParameters`)


Repository:
  rL LLVM

https://reviews.llvm.org/D32961