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

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added inline comments.


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


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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:411
+
+  switch (I->getOpcode()) {
+  case Instruction::GetElementPtr: {
----------------
arsenm wrote:
> 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
Let me take a relook at it.


================
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
----------------
arsenm wrote:
> 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.
Let me take a relook at it.


================
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;
+}
----------------
arsenm wrote:
> I don't understand what the "base type" means or why we would care
"base type" means base type of an aggregate type. I somehow assumed that information about base type of an aggregate type is required. But, let me take a relook at it.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516