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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:336
+      BasePtr->getType()->getPointerElementType(), BasePtr, LInst,
+      Twine("dssv.gep2.") + Twine(Suffix), const_cast<Instruction *>(&EI));
+
----------------
There should be no const_casts


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:478
+
+  NewParams.push_back(BasePtrTy);
+  NewParams.push_back(KernNumTy);
----------------
I think we could avoid passing the base pointer since it should always be 0. I would have to think about how to best guarantee this to codegen though


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUDeviceScopeSharedVariable.cpp:1510
+  // Collect all the amdgpu kernels defined within the current module.
+  SetVector<Function *> Kernels;
+  for (auto &F : M.functions())
----------------
hsmhsm wrote:
> arsenm wrote:
> > Don't see why this is a SetVector, the ordering shouldn't matter
> I intentionally used SetVector in order to make sure that the TRAVERSAL ORDER is preserved during multiple traversals of this set in different places. 
> 
> Is it harm to use SetVector here? Is it causes noticible performance issues? If not, why not we use it when it would provide additional benifit of preserving TRAVERSAL ORDER?
> 
Traversal order shouldn't matter though


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.h:43
   static bool EnableFixedFunctionABI;
+  static bool EnableDeviceScopeSharedVariable;
 
----------------
hsmhsm wrote:
> arsenm wrote:
> > I don't think this needs to be exposed here, there's no reason other places would need to inspect this
> This is needed in the function `AMDGPUAlwaysInlinePass.cpp`. Please look at the changes being made to this file.
This should be a required pass without an option to disable it that the pass would need to be aware of. Is this just for debug/bringup?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91516