[PATCH] D91516: [AMDGPU] Support for device scope shared variables
Mahesha S via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 8 06:37:16 PST 2021
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
More information about the llvm-commits
mailing list