[PATCH] D91516: [AMDGPU] Support for device scope shared variables

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

This is a bit different than the most recent proposal which I thought avoided the need to pass multiple arguments per kernel and allowed supporting indirect calls. I thought this was going to produce a table in constant memory containing the offsets which would be indexed instead.

Needs test with indirect calls and stored function addresses.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:163
+    auto OldCallieList = KI->second;
+    std::set<Function *> NewCallieList;
+    for (auto *OldCallie : OldCallieList) {
----------------
Spelling, Callee


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:330-332
+unsigned getFixedSizeOfTypeInBytes(Module &M, Type *Ty) {
+  return getFixedSizeOfTypeInBits(M, Ty) / 8;
+}
----------------
getTypeStoreSize


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:411
+
+  switch (I->getOpcode()) {
+  case Instruction::GetElementPtr: {
----------------
I don't see why you need to specially handle all user types. You aren't changing the type, so you should be able to do a simple replaceAllUsesWith


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:632-635
+    auto *CI = dyn_cast<CallInst>(U);
+#ifndef NDEBUG
+    assert(CI && "Valid call instruction expected");
+#endif
----------------
Just use cast<>. This will also fail on cases where you are storing the address of the function, so I don't think this is whaty ou want.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:930-934
+static Type *getLDSBaseType(Type *LDSTy) {
+  if (auto *ArrTy = dyn_cast<ArrayType>(LDSTy))
+    return getLDSBaseType(ArrTy->getElementType());
+  return LDSTy;
+}
----------------
I don't understand what the "base type" means or why we would care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:984-985
+  // floating-point types.
+  if (!LDSBaseType->isIntegerTy() && !LDSBaseType->isFloatingPointTy())
+    llvm_unreachable("Not Implemented.");
+
----------------
Memory types don't matter, there should be no need to consider it at all


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516