[PATCH] D139433: [amdgpu] Reimplement LDS lowering

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:176-185
+bool isKernelLDS(const Function *F) {
+  // Some weirdness here. AMDGPU::isKernelCC does not call into
+  // AMDGPU::isKernel with the calling conv, it instead calls into
+  // isModuleEntryFunction which returns true for more calling conventions
+  // than AMDGPU::isKernel does. There's a FIXME on AMDGPU::isKernel.
+  // There's also a test that checks that the LDS lowering does not hit on
+  // a graphics shader, denoted amdgpu_ps, so stay with the limited case.
----------------
Can you fix this situation in a later commit?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:398
+                                /* IgnoreCallbackUses */ false,
+                                /* IgnoreAssumeLikeCalls */ false,
+                                /* IgnoreLLVMUsed */ true,
----------------
Not sure why this is false, change in a separate commit?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:485
+
+  // remap from lds global to a constantexpr gep to where it has been moved to
+  // for each kernel
----------------
Capitalize


================
Comment at: llvm/test/CodeGen/AMDGPU/lower-module-lds-single-var-ambiguous.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -mtriple=amdgcn-- -amdgpu-lower-module-lds < %s --amdgpu-lower-module-lds-strategy=module | FileCheck -check-prefixes=CHECK,M_OR_HY %s
+; RUN: opt -S -mtriple=amdgcn-- -amdgpu-lower-module-lds < %s --amdgpu-lower-module-lds-strategy=table | FileCheck -check-prefixes=CHECK,TABLE %s
----------------
Can you use -passes?


================
Comment at: llvm/test/CodeGen/AMDGPU/lower-module-lds-single-var-ambiguous.ll:17
+;
+  %ld = load i8, i8 addrspace(3)* @kernel.lds
+  %mul = mul i8 %ld, 2
----------------
New tests should use opaque pointers


================
Comment at: llvm/test/CodeGen/AMDGPU/lower-module-lds-single-var-unambiguous.ll:20
+;
+  %ld = load i8, i8 addrspace(3)* @k0.lds
+  %mul = mul i8 %ld, 2
----------------
Opaque pointers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139433