[PATCH] D102954: [AMDGPU] Lower kernel LDS into a sorted structure

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:228
 
+    Twine VarName(F ? "llvm.amdgcn.kernel." + Twine(F->getName() + ".lds")
+                    : "llvm.amdgcn.module.lds");
----------------
hsmhsm wrote:
> There is some problem here - because of this, llvm -lit tests including your own newly added tests are failing.
> 
> After changing
> 
> Twine VarName(F ? "llvm.amdgcn.kernel." + Twine(F->getName() + ".lds") : "llvm.amdgcn.module.lds");
> 
> To
> 
> Twine VarName(F ? Twine("llvm.amdgcn.kernel.") + Twine(F->getName()) + Twine(".lds")  : "llvm.amdgcn.module.lds");
> 
> it works.
> 
clang-tidy also warns about local twine variable. Switched to std::string.


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPULDSUtils.cpp:37
+
+  if (auto *C = dyn_cast<ConstantExpr>(U)) {
+    return all_of(U->users(), [F](const User *U) {
----------------
JonChesterfield wrote:
> Is this safe (enough) for space? Wonder if it's another place for a heap allocated worklist
It goes up the users and bails on a first non ConstantExpr use. The call stack is as deep as much nested ConstantExprs we have. To run into a problem we would need to have a ConstExpr of a form like gep(gep(gep(gep(...)))) nested really deep. If you still feel I need to switch to a worklist I will do it, it just looks unnecessary here to me.


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

https://reviews.llvm.org/D102954