[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:70
+//
+// Is one really exist?
+//
----------------
arsenm wrote:
> Don't understand this comment
Removed the confusing comment


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:198
+                "implement-amdgpu-device-scope-shared-variable",
+                "Implement AMDPGU Device Scope Shared Variable",
+                false /*only look at the cfg*/, false /*analysis pass*/)
----------------
arsenm wrote:
> Spelling AMDPGU
Taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:201-207
+static unsigned getTypeStoreSizeInBits(Module &M, Type *Ty) {
+  return M.getDataLayout().getTypeSizeInBits(Ty).getFixedSize();
+}
+
+static unsigned getTypeStoreSizeInBytes(Module &M, Type *Ty) {
+  return getTypeStoreSizeInBits(M, Ty) / 8;
+}
----------------
arsenm wrote:
> These functions only make this harder to read. You're losing value vs. just directly using the datalayout functions
Taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:281
+    // Cast away const-ness from `U`.
+    User *UU = const_cast<User *>(U);
+
----------------
arsenm wrote:
> Should not const_cast
Taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:469-470
+static void copyDataFromOldToNewCallSite(CallInst *OldCI, CallInst *NewCI) {
+  // TODO: Why copyMetadata() not copying meta data. I see metadat associated
+  // with CI, but it is not copied to NewCI. CI->hasMetadata() is false, why?
+  NewCI->copyMetadata(*OldCI);
----------------
arsenm wrote:
> Comment doesn't make sense
Taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:573
+                 ValueMap<Function *, Function *> &NewCalleeToOldCallee) {
+  // We are only interested in the inctructions within kernel or within new
+  // cloned functions.
----------------
arsenm wrote:
> Spelling inctructions
Taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:575
+  // cloned functions.
+#ifndef NDEBUG
+  assert(I && "Valid instruction expected\n");
----------------
arsenm wrote:
> Redundant ifndef
Taken care


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:1167
+
+  // Create a single contigeous LDS global layout for each kernel.
+  std::map<Function *, std::map<GlobalVariable *, uint64_t>> KernelToLDSOffset;
----------------
arsenm wrote:
> Spelling contigeous
Taken care


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516