[PATCH] D82496: [amdgpu] Add codegen support for HIP dynamic shared memory.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

I don't think this actually works since you could have multiple 0 sized objects, and they would both get the same address. I think this has to be an external relocation



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5523
+        GV->hasExternalLinkage()) {
+      ArrayType *ATy = dyn_cast<ArrayType>(GV->getValueType());
+      if (ATy && ATy->getNumElements() == 0) {
----------------
hliao wrote:
> arsenm wrote:
> > Why does it specifically need to be a 0 sized array? I think this would depend purely on the linkage, or treat any 0 sized type the same way
> That's the only syntax accepted under HIP clang. At least, zero-sized types should be checked to ensure developers understand the usage of the dynamic shared memory.
That's a bit too much HIP specific logic. Also what does this do if there are more than one? How can these return different addresses?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:5527
+        return SDValue(
+            DAG.getMachineNode(AMDGPU::GET_GROUPSTATICSIZE, DL, PtrVT), 0);
+      }
----------------
hliao wrote:
> scchan wrote:
> > don't you need to make sure whether the static size would give you an offset with the correct alignment?
> I remember that size should be always DWORD aligned. Let me check the code calculated that.
The global has an explicit alignment that needs to be respected


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82496