[PATCH] D144221: [amdgpu][nfc] Replace ad hoc LDS frame recalculation with absolute_symbol MD

Mon Feb 20 12:59:13 PST 2023

arsenm added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.cpp:124
+  auto Type = M->getDataLayout().getIntPtrType(Ctx, AMDGPUAS::LOCAL_ADDRESS);
+  GV->setMetadata(LLVMContext::MD_absolute_symbol,
+                  MDNode::get(Ctx, ConstantAsMetadata::get(
----------------
JonChesterfield wrote:
> arsenm wrote:
> > Changing the IR in a codegen pass is bad. Can you set this earlier?
> Setting metadata on the IR during codegen is not pretty but it's also inconsequential. The frame calculation this deletes is also quite ugly though.
> 
> For the existing cases we can set the metadata in the IR lowering (carefully, but accurately enough). It'll make changes to promote alloca fragile. That won't work for external/dynamic LDS as we don't know that address until the end of ISel.
> 
> So yes... if we want a hard no on setting metadata to record information learned during codegen, we can avoid it. Basically by lifting the remaining LDS lowering currently done by codegen wholly out of codegen. That'll change how dynamic LDS is handled from kernels which I was hoping to avoid.
> 
> If we're up for tagging variables with metadata at this point we can write the address chosen for dynamic LDS variables the same way and the lowerConstant path added here will do the right thing for them.
So you're implying the promote alloca introduced LDS still has possibly unpredictable offsets assigned for it?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144221