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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:90
+  std::string KernelLDSName = "llvm.amdgcn.kernel.";
+  KernelLDSName += F.getName();
+  KernelLDSName += ".lds";
----------------
JonChesterfield wrote:
> 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.
You could do what the final emission does, and use the name out of Mangler::getNameWithPrefix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127052