[PATCH] D127052: [amdgpu][nfc] Allocate kernel-specific LDS struct deterministically

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:90
+  std::string KernelLDSName = "llvm.amdgcn.kernel.";
+  KernelLDSName += F.getName();
+  KernelLDSName += ".lds";
----------------
arsenm wrote:
> JonChesterfield wrote:
> > arsenm wrote:
> > > Need to be careful about multiple anonymous functions
> > How so? By the time they're functions in IR they have unique names as far as I know. If they don't, the current lowering is already broken for them as it assumes a struct can be uniquely associated with a function by deriving the variable name from the function name
> They don't have to have a name at all. If you have kernels "@0" and "@1" they'll get the same thing here
That's exciting. Broken at present, AMDGPULowerModuleLDSPass creates the kernel-specific variable by passing `Twine("llvm.amdgcn.kernel.") + F->getName() + ".lds"` to StructType::Create() with a suffix ".t".

I'd be inclined to say the right fix for that is to hard error on them on the basis that anonymous kernels are difficult to invoke by name, but failing that we could assign them names in LowerModuleLDS.

Or we could stop using string equivalence to bind a function and a struct together and do something along the lines of metadata instead which should be more robust to things like renaming the kernel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127052