[PATCH] D131276: AMDGPU: Implicit kernel arguments related optimization when uniform-workgroup-size=true

[Changpeng Fang via Phabricator via llvm-commits]

cfang marked 6 inline comments as done.
cfang added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:47-49
+  BLOCK_COUNT_X = 0,
+  BLOCK_COUNT_Y = 4,
+  BLOCK_COUNT_Z = 8,
----------------
arsenm wrote:
> Should be named HIDDEN_BLOCK_*?
Ok, named with hidden_ prefix to reflect the field names. Thanks.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:91
 
-  Value *WorkGroupSizeX = nullptr;
-  Value *WorkGroupSizeY = nullptr;
-  Value *WorkGroupSizeZ = nullptr;
-
-  Value *GridSizeX = nullptr;
-  Value *GridSizeY = nullptr;
-  Value *GridSizeZ = nullptr;
+  Value *BlockCounts[3] = {nullptr, nullptr, nullptr};
+  Value *GroupSizes[3]  = {nullptr, nullptr, nullptr};
----------------
arsenm wrote:
> Hidden?
These are just temporary variable names, and some of them are shared with pre-v5 implementation. I am thinking it
is better not to add hidden_ prefix.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:336-337
 
-  Function *DispatchPtr = M.getFunction(DispatchPtrName);
-  if (!DispatchPtr) // Dispatch ptr not used.
+    if (!DispatchPtr) // Dispatch ptr not used.
     return false;
 
----------------
arsenm wrote:
> Looks like something weird is happening with the return indentation
Fixed. 


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:376
 
+  bool IsV5OrAbove = AMDGPU::getAmdhsaCodeObjectVersion() >= 5;
   for (Instruction &I : instructions(F)) {
----------------
arsenm wrote:
> This isn't reading the IR/module flag
Waiting for clang to generate a complete code object versions (only v5 at this moment). This is be updated with a separate task (patch).


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerKernelAttributes.cpp:381
+        processUse(CI, true);
+      if (!IsV5OrAbove && CI->getCalledFunction() == DispatchPtr)
+        processUse(CI, false);
----------------
arsenm wrote:
> else if
Done. Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131276/new/

https://reviews.llvm.org/D131276