[PATCH] D138141: [amdgpu] Reimplement LDS lowering

[Matt Arsenault via Phabricator via llvm-commits]

arsenm requested changes to this revision.
arsenm added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:385
+
+      std::string varname = GV.getName().str();
+
----------------
Don't need std::string, but this is also unused?



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:391
+          Function *F = I->getFunction();
+          std::string funcname = F->getName().str();
+          if (isKernelLDS(F)) {
----------------
Don't need std::string, but this is also unused?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:518
+
+    auto I16 = Type::getInt16Ty(Ctx);
+    auto I32 = Type::getInt32Ty(Ctx);
----------------
Type *I16


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:573
+        M, AllKernelsOffsetsType, true, GlobalValue::InternalLinkage, init,
+        "llvm.amdgcn.lds_offset_table", nullptr, GlobalValue::NotThreadLocal,
+        AMDGPUAS::CONSTANT_ADDRESS);
----------------
Not sure if this is just copying something existing, but I think it would be preferably to consistently use . as the word separator instead of mixing in _


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:655
+    DenseSet<Function *> KernelSet;
+    if (VariableSet.empty()) return KernelSet;
+
----------------
New line


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:781
+        case LoweringKind::Opts::unknown:
+          __builtin_unreachable(); // would report fatal error from LoweringKind
+
----------------
llvm_unreachable


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:795-796
           } else {
-            // However if we are certain this kernel cannot call a function that
-            // requires module LDS, annotate the kernel so the backend can elide
-            // the allocation without repeating callgraph walks.
-            Func.addFnAttr("amdgpu-elide-module-lds");
+            report_fatal_error("Cannot lower LDS to kernel access as it is "
+                               "reachable from multiple kernels");
           }
----------------
Probably should promote this to a DiagnosticInfoUnsupported


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:161-190
+  static constexpr const char name[] = "amdgpu-lower-module-lds-strategy";
+
+  enum Opts { module, table, kernel, hybrid, unknown };
+
+  Opts Value = Opts::module;
+  operator Opts() const { return Value; }
+
----------------
JonChesterfield wrote:
> arsenm wrote:
> > You're re-inventing cl::opt with clEnumVal
> So I was. Interesting. Changed to use clEnumValN (to preserve the namespacing at the use sites). 
> 
> Sadly the test case lds-reject-unknown-lowering-kind.ll - updated with the new error string - is now refusing to run under check-llvm despite running fine in a shell. That coverage seems less important when using a ready-made parser so I've deleted the test.
> 
I'm not seeing this change here?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:532
+      } else {
+        Elements.push_back(UndefValue::get(I16));
+      }
----------------
JonChesterfield wrote:
> nlopes wrote:
> > Please use PoisonValue here if possible. We are trying to remove undef values, and thus we want to avoid adding new uses that can be easily replaced with poison.
> > Thank you!
> Whether this can be easily replaced with poison depends on how the rest of the toolchain deals with poison. The string replacement is fine, it's whether it then blows up under testing due to interaction with the rest of the compiler that encourages caution.
> 
> The semantics of poison fit this behaviour - if this value is read, the compiler has a bug or runtime behaviour has diverged from that compiler's expectation. I would therefore like to change from undef to poison after this lands, so that if it does fail testing we have a much easier time triaging.
Just change to poison. Nothing we're doing in the backend is aware enough of the difference (currently poison just gets selected to the same implicit_def anyway, although that's kind of broken)


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp:640
+        Value *replacement =
+            Builder.CreateIntToPtr(extended, GV->getType(), GV->getName());
+
----------------
JonChesterfield wrote:
> arsenm wrote:
> > Can you add an assert somewhere that the maximum LDS size fits in 16-bits? I'm also wondering if we should just make LDS pointers 16-bit with 4 byte alignment, or add a 16-bit address space for this
> The backend currently errors on that in user-facing fashion. A check here will be slightly less accurate than in the back end because this runs before promote alloca puts more things into LDS and I'm not sure we have more information to provide a better report either - what sort of error message do you have in mind?
> 
> I suppose the current code path would truncate the offsets to i16 before the back end crashes, but that seems harmless. We'd have slightly cleaner tests with a native sixteen bit type but there's not much in it.
I don't mean here, I mean somewhere. Promote alloca also shouldn't be pushing the usage up into something unsupportable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138141