LLVMContext: add getSyncScopeName() to lookup individual scope name #109484

gonzalobg · 2024-09-20T21:52:00Z

This PR adds a getSyncScopeString(Id) API to LLVMContext that returns the StringRef for that ID, if any.
It uses it to improve error reporting in the NVPTX backend.

Draft: it builds on #106101 . Will change from Draft to non-Draft once that one is merged.
This PR's commits start at [LLVM] Add LLVMContext API to print one SyncScope ID string.

gonzalobg · 2024-09-20T21:53:36Z

@Artem-B @arsenm I think you both may have enough context to review.

github-actions · 2024-09-20T21:56:34Z

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm

Lots of unrelated commits, but yes the existing API is clumsy:

llvm-project/llvm/lib/Target/AMDGPU/SIISelLowering.cpp

Line 16146 in 0b08747

Ctx.getSyncScopeNames(SSNs);

llvmbot · 2024-09-23T20:33:47Z

@llvm/pr-subscribers-backend-nvptx

Author: None (gonzalobg)

Changes

This PR adds a getSyncScopeString(Id) API to LLVMContext that returns the StringRef for that ID, if any.
It uses it to improve error reporting in the NVPTX backend.

Draft: it builds on #106101 . Will change from Draft to non-Draft once that one is merged.
This PR's commits start at [LLVM] Add LLVMContext API to print one SyncScope ID string.

Full diff: https://github.com/llvm/llvm-project/pull/109484.diff

7 Files Affected:

(modified) llvm/include/llvm/IR/LLVMContext.h (+4)
(modified) llvm/lib/IR/LLVMContext.cpp (+4)
(modified) llvm/lib/IR/LLVMContextImpl.cpp (+8)
(modified) llvm/lib/IR/LLVMContextImpl.h (+4)
(modified) llvm/lib/Target/AMDGPU/SIISelLowering.cpp (+2-5)
(modified) llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp (+6-8)
(modified) llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h (+1)

diff --git a/llvm/include/llvm/IR/LLVMContext.h b/llvm/include/llvm/IR/LLVMContext.h
index 6ffa2bdaa319a7..19f6a91392ae36 100644
--- a/llvm/include/llvm/IR/LLVMContext.h
+++ b/llvm/include/llvm/IR/LLVMContext.h
@@ -130,6 +130,10 @@ class LLVMContext {
   /// scope names are ordered by increasing synchronization scope IDs.
   void getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const;
 
+  /// getSyncScopeName - Returns the name of a SyncScope::ID
+  /// registered with LLVMContext, if any.
+  std::optional<StringRef> getSyncScopeName(SyncScope::ID Id) const;
+  
   /// Define the GC for a function
   void setGC(const Function &Fn, std::string GCName);
 
diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
index c0fee93a233808..e897cd2c9fd858 100644
--- a/llvm/lib/IR/LLVMContext.cpp
+++ b/llvm/lib/IR/LLVMContext.cpp
@@ -330,6 +330,10 @@ void LLVMContext::getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const {
   pImpl->getSyncScopeNames(SSNs);
 }
 
+std::optional<StringRef> LLVMContext::getSyncScopeName(SyncScope::ID Id) const {
+  return pImpl->getSyncScopeName(Id);
+}
+
 void LLVMContext::setGC(const Function &Fn, std::string GCName) {
   pImpl->GCNames[&Fn] = std::move(GCName);
 }
diff --git a/llvm/lib/IR/LLVMContextImpl.cpp b/llvm/lib/IR/LLVMContextImpl.cpp
index 4f1ef8cec32133..424a04ba9659b9 100644
--- a/llvm/lib/IR/LLVMContextImpl.cpp
+++ b/llvm/lib/IR/LLVMContextImpl.cpp
@@ -244,6 +244,14 @@ void LLVMContextImpl::getSyncScopeNames(
     SSNs[SSE.second] = SSE.first();
 }
 
+std::optional<StringRef> LLVMContextImpl::getSyncScopeName(SyncScope::ID Id) const {
+  for (const auto &SSE : SSC) {
+    if (SSE.second != Id) continue;
+    return SSE.first();
+  }
+  return std::nullopt;
+}
+
 /// Gets the OptPassGate for this LLVMContextImpl, which defaults to the
 /// singleton OptBisect if not explicitly set.
 OptPassGate &LLVMContextImpl::getOptPassGate() const {
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index e76f004b590efe..a80fb39ce2eafb 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -1665,6 +1665,10 @@ class LLVMContextImpl {
   /// scope names are ordered by increasing synchronization scope IDs.
   void getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const;
 
+  /// getSyncScopeName - Returns the name of a SyncScope::ID
+  /// registered with LLVMContext, if any.
+  std::optional<StringRef> getSyncScopeName(SyncScope::ID Id) const;
+  
   /// Maintain the GC name for each function.
   ///
   /// This saves allocating an additional word in Function for programs which
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index a9754ba357893f..970abe4434ddfc 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -16142,11 +16142,8 @@ static bool atomicIgnoresDenormalModeOrFPModeIsFTZ(const AtomicRMWInst *RMW) {
 
 static OptimizationRemark emitAtomicRMWLegalRemark(const AtomicRMWInst *RMW) {
   LLVMContext &Ctx = RMW->getContext();
-  SmallVector<StringRef> SSNs;
-  Ctx.getSyncScopeNames(SSNs);
-  StringRef MemScope = SSNs[RMW->getSyncScopeID()].empty()
-                           ? "system"
-                           : SSNs[RMW->getSyncScopeID()];
+  StringRef SS = Ctx.getSyncScopeName(RMW->getSyncScopeID());
+  StringRef  MemScope = SS.empty()? StringRef("system") : SS;
 
   return OptimizationRemark(DEBUG_TYPE, "Passed", RMW)
          << "Hardware instruction generated for atomic "
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
index 56c96ea943b89d..3c9ca187109281 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
+++ b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
@@ -4176,7 +4176,7 @@ bool NVPTXDAGToDAGISel::tryFence(SDNode *N) {
   return true;
 }
 
-NVPTXScopes::NVPTXScopes(LLVMContext &C) {
+NVPTXScopes::NVPTXScopes(LLVMContext &C) : CTX(&C) {
   Scopes[C.getOrInsertSyncScopeID("singlethread")] = NVPTX::Scope::Thread;
   Scopes[C.getOrInsertSyncScopeID("")] = NVPTX::Scope::System;
   Scopes[C.getOrInsertSyncScopeID("block")] = NVPTX::Scope::Block;
@@ -4190,13 +4190,11 @@ NVPTX::Scope NVPTXScopes::operator[](SyncScope::ID ID) const {
                      "NVPTXScopes::operator[]");
 
   auto S = Scopes.find(ID);
-  if (S == Scopes.end()) {
-    // TODO:
-    // - Add API to LLVMContext to get the name of a single scope.
-    // - Use that API here to print an error containing the name
-    //   of this Unknown ID.
-    report_fatal_error(formatv("Could not find scope ID={}.", int(ID)));
-  }
+  if (S == Scopes.end())
+    report_fatal_error(
+        formatv("Could not find scope ID={} with name \"{}\".", int(ID),
+		CTX->getSyncScopeName(ID).value_or("unknown")));
+
   return S->second;
 }
 
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
index c128c082c29837..f925fc67fbccb7 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
+++ b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
@@ -35,6 +35,7 @@ struct NVPTXScopes {
 
 private:
   SmallMapVector<SyncScope::ID, NVPTX::Scope, 8> Scopes{};
+  LLVMContext *CTX = nullptr;
 };
 
 class LLVM_LIBRARY_VISIBILITY NVPTXDAGToDAGISel : public SelectionDAGISel {

llvmbot · 2024-09-23T20:33:48Z

@llvm/pr-subscribers-backend-amdgpu

Author: None (gonzalobg)

Changes

This PR adds a getSyncScopeString(Id) API to LLVMContext that returns the StringRef for that ID, if any.
It uses it to improve error reporting in the NVPTX backend.

Draft: it builds on #106101 . Will change from Draft to non-Draft once that one is merged.
This PR's commits start at [LLVM] Add LLVMContext API to print one SyncScope ID string.

Full diff: https://github.com/llvm/llvm-project/pull/109484.diff

7 Files Affected:

(modified) llvm/include/llvm/IR/LLVMContext.h (+4)
(modified) llvm/lib/IR/LLVMContext.cpp (+4)
(modified) llvm/lib/IR/LLVMContextImpl.cpp (+8)
(modified) llvm/lib/IR/LLVMContextImpl.h (+4)
(modified) llvm/lib/Target/AMDGPU/SIISelLowering.cpp (+2-5)
(modified) llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp (+6-8)
(modified) llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h (+1)

diff --git a/llvm/include/llvm/IR/LLVMContext.h b/llvm/include/llvm/IR/LLVMContext.h
index 6ffa2bdaa319a7..19f6a91392ae36 100644
--- a/llvm/include/llvm/IR/LLVMContext.h
+++ b/llvm/include/llvm/IR/LLVMContext.h
@@ -130,6 +130,10 @@ class LLVMContext {
   /// scope names are ordered by increasing synchronization scope IDs.
   void getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const;
 
+  /// getSyncScopeName - Returns the name of a SyncScope::ID
+  /// registered with LLVMContext, if any.
+  std::optional<StringRef> getSyncScopeName(SyncScope::ID Id) const;
+  
   /// Define the GC for a function
   void setGC(const Function &Fn, std::string GCName);
 
diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
index c0fee93a233808..e897cd2c9fd858 100644
--- a/llvm/lib/IR/LLVMContext.cpp
+++ b/llvm/lib/IR/LLVMContext.cpp
@@ -330,6 +330,10 @@ void LLVMContext::getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const {
   pImpl->getSyncScopeNames(SSNs);
 }
 
+std::optional<StringRef> LLVMContext::getSyncScopeName(SyncScope::ID Id) const {
+  return pImpl->getSyncScopeName(Id);
+}
+
 void LLVMContext::setGC(const Function &Fn, std::string GCName) {
   pImpl->GCNames[&Fn] = std::move(GCName);
 }
diff --git a/llvm/lib/IR/LLVMContextImpl.cpp b/llvm/lib/IR/LLVMContextImpl.cpp
index 4f1ef8cec32133..424a04ba9659b9 100644
--- a/llvm/lib/IR/LLVMContextImpl.cpp
+++ b/llvm/lib/IR/LLVMContextImpl.cpp
@@ -244,6 +244,14 @@ void LLVMContextImpl::getSyncScopeNames(
     SSNs[SSE.second] = SSE.first();
 }
 
+std::optional<StringRef> LLVMContextImpl::getSyncScopeName(SyncScope::ID Id) const {
+  for (const auto &SSE : SSC) {
+    if (SSE.second != Id) continue;
+    return SSE.first();
+  }
+  return std::nullopt;
+}
+
 /// Gets the OptPassGate for this LLVMContextImpl, which defaults to the
 /// singleton OptBisect if not explicitly set.
 OptPassGate &LLVMContextImpl::getOptPassGate() const {
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index e76f004b590efe..a80fb39ce2eafb 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -1665,6 +1665,10 @@ class LLVMContextImpl {
   /// scope names are ordered by increasing synchronization scope IDs.
   void getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const;
 
+  /// getSyncScopeName - Returns the name of a SyncScope::ID
+  /// registered with LLVMContext, if any.
+  std::optional<StringRef> getSyncScopeName(SyncScope::ID Id) const;
+  
   /// Maintain the GC name for each function.
   ///
   /// This saves allocating an additional word in Function for programs which
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index a9754ba357893f..970abe4434ddfc 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -16142,11 +16142,8 @@ static bool atomicIgnoresDenormalModeOrFPModeIsFTZ(const AtomicRMWInst *RMW) {
 
 static OptimizationRemark emitAtomicRMWLegalRemark(const AtomicRMWInst *RMW) {
   LLVMContext &Ctx = RMW->getContext();
-  SmallVector<StringRef> SSNs;
-  Ctx.getSyncScopeNames(SSNs);
-  StringRef MemScope = SSNs[RMW->getSyncScopeID()].empty()
-                           ? "system"
-                           : SSNs[RMW->getSyncScopeID()];
+  StringRef SS = Ctx.getSyncScopeName(RMW->getSyncScopeID());
+  StringRef  MemScope = SS.empty()? StringRef("system") : SS;
 
   return OptimizationRemark(DEBUG_TYPE, "Passed", RMW)
          << "Hardware instruction generated for atomic "
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
index 56c96ea943b89d..3c9ca187109281 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
+++ b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
@@ -4176,7 +4176,7 @@ bool NVPTXDAGToDAGISel::tryFence(SDNode *N) {
   return true;
 }
 
-NVPTXScopes::NVPTXScopes(LLVMContext &C) {
+NVPTXScopes::NVPTXScopes(LLVMContext &C) : CTX(&C) {
   Scopes[C.getOrInsertSyncScopeID("singlethread")] = NVPTX::Scope::Thread;
   Scopes[C.getOrInsertSyncScopeID("")] = NVPTX::Scope::System;
   Scopes[C.getOrInsertSyncScopeID("block")] = NVPTX::Scope::Block;
@@ -4190,13 +4190,11 @@ NVPTX::Scope NVPTXScopes::operator[](SyncScope::ID ID) const {
                      "NVPTXScopes::operator[]");
 
   auto S = Scopes.find(ID);
-  if (S == Scopes.end()) {
-    // TODO:
-    // - Add API to LLVMContext to get the name of a single scope.
-    // - Use that API here to print an error containing the name
-    //   of this Unknown ID.
-    report_fatal_error(formatv("Could not find scope ID={}.", int(ID)));
-  }
+  if (S == Scopes.end())
+    report_fatal_error(
+        formatv("Could not find scope ID={} with name \"{}\".", int(ID),
+		CTX->getSyncScopeName(ID).value_or("unknown")));
+
   return S->second;
 }
 
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
index c128c082c29837..f925fc67fbccb7 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
+++ b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
@@ -35,6 +35,7 @@ struct NVPTXScopes {
 
 private:
   SmallMapVector<SyncScope::ID, NVPTX::Scope, 8> Scopes{};
+  LLVMContext *CTX = nullptr;
 };
 
 class LLVM_LIBRARY_VISIBILITY NVPTXDAGToDAGISel : public SelectionDAGISel {

llvmbot · 2024-09-23T20:33:48Z

@llvm/pr-subscribers-llvm-ir

Author: None (gonzalobg)

Changes

This PR adds a getSyncScopeString(Id) API to LLVMContext that returns the StringRef for that ID, if any.
It uses it to improve error reporting in the NVPTX backend.

Draft: it builds on #106101 . Will change from Draft to non-Draft once that one is merged.
This PR's commits start at [LLVM] Add LLVMContext API to print one SyncScope ID string.

Full diff: https://github.com/llvm/llvm-project/pull/109484.diff

7 Files Affected:

(modified) llvm/include/llvm/IR/LLVMContext.h (+4)
(modified) llvm/lib/IR/LLVMContext.cpp (+4)
(modified) llvm/lib/IR/LLVMContextImpl.cpp (+8)
(modified) llvm/lib/IR/LLVMContextImpl.h (+4)
(modified) llvm/lib/Target/AMDGPU/SIISelLowering.cpp (+2-5)
(modified) llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp (+6-8)
(modified) llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h (+1)

diff --git a/llvm/include/llvm/IR/LLVMContext.h b/llvm/include/llvm/IR/LLVMContext.h
index 6ffa2bdaa319a7..19f6a91392ae36 100644
--- a/llvm/include/llvm/IR/LLVMContext.h
+++ b/llvm/include/llvm/IR/LLVMContext.h
@@ -130,6 +130,10 @@ class LLVMContext {
   /// scope names are ordered by increasing synchronization scope IDs.
   void getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const;
 
+  /// getSyncScopeName - Returns the name of a SyncScope::ID
+  /// registered with LLVMContext, if any.
+  std::optional<StringRef> getSyncScopeName(SyncScope::ID Id) const;
+  
   /// Define the GC for a function
   void setGC(const Function &Fn, std::string GCName);
 
diff --git a/llvm/lib/IR/LLVMContext.cpp b/llvm/lib/IR/LLVMContext.cpp
index c0fee93a233808..e897cd2c9fd858 100644
--- a/llvm/lib/IR/LLVMContext.cpp
+++ b/llvm/lib/IR/LLVMContext.cpp
@@ -330,6 +330,10 @@ void LLVMContext::getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const {
   pImpl->getSyncScopeNames(SSNs);
 }
 
+std::optional<StringRef> LLVMContext::getSyncScopeName(SyncScope::ID Id) const {
+  return pImpl->getSyncScopeName(Id);
+}
+
 void LLVMContext::setGC(const Function &Fn, std::string GCName) {
   pImpl->GCNames[&Fn] = std::move(GCName);
 }
diff --git a/llvm/lib/IR/LLVMContextImpl.cpp b/llvm/lib/IR/LLVMContextImpl.cpp
index 4f1ef8cec32133..424a04ba9659b9 100644
--- a/llvm/lib/IR/LLVMContextImpl.cpp
+++ b/llvm/lib/IR/LLVMContextImpl.cpp
@@ -244,6 +244,14 @@ void LLVMContextImpl::getSyncScopeNames(
     SSNs[SSE.second] = SSE.first();
 }
 
+std::optional<StringRef> LLVMContextImpl::getSyncScopeName(SyncScope::ID Id) const {
+  for (const auto &SSE : SSC) {
+    if (SSE.second != Id) continue;
+    return SSE.first();
+  }
+  return std::nullopt;
+}
+
 /// Gets the OptPassGate for this LLVMContextImpl, which defaults to the
 /// singleton OptBisect if not explicitly set.
 OptPassGate &LLVMContextImpl::getOptPassGate() const {
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index e76f004b590efe..a80fb39ce2eafb 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -1665,6 +1665,10 @@ class LLVMContextImpl {
   /// scope names are ordered by increasing synchronization scope IDs.
   void getSyncScopeNames(SmallVectorImpl<StringRef> &SSNs) const;
 
+  /// getSyncScopeName - Returns the name of a SyncScope::ID
+  /// registered with LLVMContext, if any.
+  std::optional<StringRef> getSyncScopeName(SyncScope::ID Id) const;
+  
   /// Maintain the GC name for each function.
   ///
   /// This saves allocating an additional word in Function for programs which
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index a9754ba357893f..970abe4434ddfc 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -16142,11 +16142,8 @@ static bool atomicIgnoresDenormalModeOrFPModeIsFTZ(const AtomicRMWInst *RMW) {
 
 static OptimizationRemark emitAtomicRMWLegalRemark(const AtomicRMWInst *RMW) {
   LLVMContext &Ctx = RMW->getContext();
-  SmallVector<StringRef> SSNs;
-  Ctx.getSyncScopeNames(SSNs);
-  StringRef MemScope = SSNs[RMW->getSyncScopeID()].empty()
-                           ? "system"
-                           : SSNs[RMW->getSyncScopeID()];
+  StringRef SS = Ctx.getSyncScopeName(RMW->getSyncScopeID());
+  StringRef  MemScope = SS.empty()? StringRef("system") : SS;
 
   return OptimizationRemark(DEBUG_TYPE, "Passed", RMW)
          << "Hardware instruction generated for atomic "
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
index 56c96ea943b89d..3c9ca187109281 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
+++ b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp
@@ -4176,7 +4176,7 @@ bool NVPTXDAGToDAGISel::tryFence(SDNode *N) {
   return true;
 }
 
-NVPTXScopes::NVPTXScopes(LLVMContext &C) {
+NVPTXScopes::NVPTXScopes(LLVMContext &C) : CTX(&C) {
   Scopes[C.getOrInsertSyncScopeID("singlethread")] = NVPTX::Scope::Thread;
   Scopes[C.getOrInsertSyncScopeID("")] = NVPTX::Scope::System;
   Scopes[C.getOrInsertSyncScopeID("block")] = NVPTX::Scope::Block;
@@ -4190,13 +4190,11 @@ NVPTX::Scope NVPTXScopes::operator[](SyncScope::ID ID) const {
                      "NVPTXScopes::operator[]");
 
   auto S = Scopes.find(ID);
-  if (S == Scopes.end()) {
-    // TODO:
-    // - Add API to LLVMContext to get the name of a single scope.
-    // - Use that API here to print an error containing the name
-    //   of this Unknown ID.
-    report_fatal_error(formatv("Could not find scope ID={}.", int(ID)));
-  }
+  if (S == Scopes.end())
+    report_fatal_error(
+        formatv("Could not find scope ID={} with name \"{}\".", int(ID),
+		CTX->getSyncScopeName(ID).value_or("unknown")));
+
   return S->second;
 }
 
diff --git a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
index c128c082c29837..f925fc67fbccb7 100644
--- a/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
+++ b/llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.h
@@ -35,6 +35,7 @@ struct NVPTXScopes {
 
 private:
   SmallMapVector<SyncScope::ID, NVPTX::Scope, 8> Scopes{};
+  LLVMContext *CTX = nullptr;
 };
 
 class LLVM_LIBRARY_VISIBILITY NVPTXDAGToDAGISel : public SelectionDAGISel {

Artem-B

The commit could use a better title. The point is that we're introducing a new API, so something along the lines of LLVMContext: add getSyncScopeName() to lookup individual scope name would work better than the one that mentions printing.

The changes that take advantage of the new API I would put into a separate patch.
The change in SIISelLowering.cpp could be considered to be a straightforward example of the new API use, and can probably go along with the API change.

However the change to NVPTX adds the new functionality and should probably be split into a separate patch.

The changes themselves look good, so it's just a mechanical exercise to land the changes as the minimal self-contained patches.

arsenm · 2024-09-24T04:31:24Z

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

-  StringRef MemScope = SSNs[RMW->getSyncScopeID()].empty()
-                           ? "system"
-                           : SSNs[RMW->getSyncScopeID()];
+  StringRef SS = Ctx.getSyncScopeName(RMW->getSyncScopeID());


This returns optional, but this is directly using it as StringRef?

FWIW I think having this return an empty StringRef in the unknown cases is a reasonable behavior vs. optional. I would expect unrecognized names to be treated as system

Have updated AMDGPU to map an out of bound SyncScope::ID to "" (system) with .value_or(""), but NVPTX maps out of bounds SyncScope::ID to errors, so just returning an empty string doesn't work there.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp

arsenm · 2024-09-24T11:56:43Z

llvm/lib/Target/NVPTX/NVPTXISelDAGToDAG.cpp

-    report_fatal_error(formatv("Could not find scope ID={}.", int(ID)));
-  }
+  if (S == Scopes.end())
+    report_fatal_error(formatv("Could not find scope ID={} with name \"{}\".",


If you really want this to be an error, I think this would be better to use C.emitError instead of a hard exit immediately

noted, it seems that there are a few other places in the NVPTX backend where we could use it, so will look at that in a different PR (also will move this NVPTX change to a different PR anyways per Artem feedback above).

gonzalobg · 2024-09-24T22:10:36Z

Have moved the use of this inside NVPTX to #109871, which can be merged later (will update that one when this one is merged).

gonzalobg · 2024-09-25T18:09:28Z

Can someone please merge this? (I still don't have permissions)

…lvm#109484) This PR adds a `getSyncScopeString(Id)` API to `LLVMContext` that returns the `StringRef` for that ID, if any.

arsenm reviewed Sep 21, 2024

View reviewed changes

gonzalobg force-pushed the llvm/syncscope_str branch from 1eb371d to 51d29ef Compare September 23, 2024 20:33

llvmbot added backend:AMDGPU backend:NVPTX llvm:ir labels Sep 23, 2024

gonzalobg changed the title ~~Draft: LLVMContext API to print single SyncScope ID~~ LLVMContext API to print single SyncScope ID Sep 23, 2024

gonzalobg force-pushed the llvm/syncscope_str branch from 51d29ef to a54e51e Compare September 23, 2024 21:28

Artem-B reviewed Sep 23, 2024

View reviewed changes

gonzalobg changed the title ~~LLVMContext API to print single SyncScope ID~~ LLVMContext: add getSyncScopeName() to lookup individual scope name Sep 23, 2024

gonzalobg force-pushed the llvm/syncscope_str branch from a54e51e to af76395 Compare September 23, 2024 22:48

arsenm reviewed Sep 24, 2024

View reviewed changes

gonzalobg force-pushed the llvm/syncscope_str branch from 3897087 to dd0313e Compare September 24, 2024 09:12

arsenm reviewed Sep 24, 2024

View reviewed changes

llvm/lib/Target/AMDGPU/SIISelLowering.cpp Show resolved Hide resolved

[LLVM] Add LLVMContext API to print one SyncScope ID string

2647573

gonzalobg force-pushed the llvm/syncscope_str branch from dd0313e to 29b015b Compare September 24, 2024 11:20

arsenm reviewed Sep 24, 2024

View reviewed changes

arsenm approved these changes Sep 24, 2024

View reviewed changes

[AMDGPU] NFC: get string for a single scope

301e84f

gonzalobg force-pushed the llvm/syncscope_str branch from 29b015b to 301e84f Compare September 24, 2024 22:09

Artem-B approved these changes Sep 24, 2024

View reviewed changes

Artem-B merged commit 0f52193 into llvm:main Sep 25, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LLVMContext: add getSyncScopeName() to lookup individual scope name #109484

LLVMContext: add getSyncScopeName() to lookup individual scope name #109484

gonzalobg commented Sep 20, 2024

gonzalobg commented Sep 20, 2024

github-actions bot commented Sep 20, 2024 •

edited

Loading

arsenm left a comment

llvmbot commented Sep 23, 2024

llvmbot commented Sep 23, 2024

llvmbot commented Sep 23, 2024

Artem-B left a comment

arsenm Sep 24, 2024

gonzalobg Sep 24, 2024

arsenm Sep 24, 2024

gonzalobg Sep 24, 2024 •

edited

Loading

gonzalobg commented Sep 24, 2024

gonzalobg commented Sep 25, 2024

LLVMContext: add getSyncScopeName() to lookup individual scope name #109484

LLVMContext: add getSyncScopeName() to lookup individual scope name #109484

Conversation

gonzalobg commented Sep 20, 2024

gonzalobg commented Sep 20, 2024

github-actions bot commented Sep 20, 2024 • edited Loading

arsenm left a comment

Choose a reason for hiding this comment

llvmbot commented Sep 23, 2024

llvmbot commented Sep 23, 2024

llvmbot commented Sep 23, 2024

Artem-B left a comment

Choose a reason for hiding this comment

arsenm Sep 24, 2024

Choose a reason for hiding this comment

gonzalobg Sep 24, 2024

Choose a reason for hiding this comment

arsenm Sep 24, 2024

Choose a reason for hiding this comment

gonzalobg Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

gonzalobg commented Sep 24, 2024

gonzalobg commented Sep 25, 2024

github-actions bot commented Sep 20, 2024 •

edited

Loading

gonzalobg Sep 24, 2024 •

edited

Loading