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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUMachineFunction.h:120
-  static bool isKnownAddressLDSGlobal(const GlobalVariable &GV);
-  static unsigned calculateKnownAddressOfLDSGlobal(const GlobalVariable &GV);
 
----------------
JonChesterfield wrote:
> JonChesterfield wrote:
> > Note to self - this shouldn't be dead after this patch - find out what happened to the lowering path for the currently unused "direct" scheme
> I think a block was lost in a git merge - AMDGPUIselLowering should have a block in it like:
> 
> ```
>  if (G->getAddressSpace() == AMDGPUAS::LOCAL_ADDRESS) {
>    if (!MFI->isModuleEntryFunction()) {
>      if (const GlobalVariable *GVar = dyn_cast<GlobalVariable>(GV)) {
>        if (AMDGPUMachineFunction::isKnownAddressLDSGlobal(*GVar)) {
>          unsigned Offset =
>              AMDGPUMachineFunction::calculateKnownAddressOfLDSGlobal(*GVar);
>          return DAG.getConstant(Offset, SDLoc(Op), Op.getValueType());
>        }
>      }
>    }
>  }
> ```
> 
> that allows direct access to kernel allocated variables from functions where the access is unambiguous. This highlights missing test coverage for the "kernel" lowering strategy (from IR to ISA). Bug is latent but will complicate getting rid of the calculateKnownAddressOfLDSGlobal function.
So if you set the absolute_address in the IR pass, and LowerGlobalAddress respected it, how would that be less reliable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144221