Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[GC] Use MapVector for GCStrategyMap #132729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 14, 2025
Merged

Conversation

paperchalice
Copy link
Contributor

Use MapVector so GCStrategyMap can support forward and reverse iterator, which is required in AsmPrinter.

StringMap<std::unique_ptr<GCStrategy>> StrategyMap;

class GCStrategyMap
: public MapVector<std::string, std::unique_ptr<GCStrategy>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be a bad practice here.

struct GCStrategyMap {
StringMap<std::unique_ptr<GCStrategy>> StrategyMap;

class GCStrategyMap : public MapVector<std::string, std::unique_ptr<GCStrategy>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, do not inherit from core data structures. Contain them instead.

Use `MapVector`, so `GCStrategyMap` can support forward and reverse iterator, which is required in `AsmPrinter`.
Comment on lines 185 to 187
std::unique_ptr<GCStrategy> &operator[](const std::string &GCName) {
return Strategies[GCName];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::unique_ptr<GCStrategy> &operator[](const std::string &GCName) {
return Strategies[GCName];
}
GCStrategy &operator[](const std::string &GCName) {
return *Strategies[GCName];
}

StringMap<std::unique_ptr<GCStrategy>> StrategyMap;
class GCStrategyMap {
using MapT =
MapVector<std::string, std::unique_ptr<GCStrategy>, StringMap<unsigned>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do these names come from? Can this use StringRef? Cursory search seems to suggest these are already live in the LLVMContext


bool empty() const { return Strategies.empty(); }

GCStrategy &operator[](StringRef GCName) { return *Strategies[GCName]; }
Copy link
Contributor

@arsenm arsenm Mar 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
GCStrategy &operator[](StringRef GCName) { return *Strategies[GCName]; }
const GCStrategy &operator[](StringRef GCName) const { return *Strategies.lookup(GCName); }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Const qualified version causes returning reference to local temporary object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow this, it has to be a const return?

Copy link
Contributor Author

@paperchalice paperchalice May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type of MapVector::lookup here is std::unique_ptr<GCStrategy> which is not copyable, so it is unavailable here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return value should be *lookup

@llvmbot
Copy link
Member

llvmbot commented Mar 25, 2025

@llvm/pr-subscribers-llvm-ir

Author: None (paperchalice)

Changes

Use MapVector so GCStrategyMap can support forward and reverse iterator, which is required in AsmPrinter.


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

6 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/GCMetadata.h (+33-2)
  • (modified) llvm/include/llvm/IR/GCStrategy.h (+1)
  • (modified) llvm/lib/CodeGen/GCMetadata.cpp (+10-10)
  • (modified) llvm/lib/CodeGen/ShadowStackGCLowering.cpp (+1-1)
  • (modified) llvm/unittests/CodeGen/CMakeLists.txt (+1)
  • (added) llvm/unittests/CodeGen/GCMetadata.cpp (+84)
diff --git a/llvm/include/llvm/CodeGen/GCMetadata.h b/llvm/include/llvm/CodeGen/GCMetadata.h
index ca6a511185c7c..a3b284ef0ca30 100644
--- a/llvm/include/llvm/CodeGen/GCMetadata.h
+++ b/llvm/include/llvm/CodeGen/GCMetadata.h
@@ -33,6 +33,7 @@
 #define LLVM_CODEGEN_GCMETADATA_H
 
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
@@ -151,15 +152,45 @@ class GCFunctionInfo {
   size_t live_size(const iterator &p) const { return roots_size(); }
 };
 
-struct GCStrategyMap {
-  StringMap<std::unique_ptr<GCStrategy>> StrategyMap;
+class GCStrategyMap {
+  using MapT =
+      MapVector<StringRef, std::unique_ptr<GCStrategy>, StringMap<unsigned>>;
+  MapT Strategies;
 
+public:
   GCStrategyMap() = default;
   GCStrategyMap(GCStrategyMap &&) = default;
 
   /// Handle invalidation explicitly.
   bool invalidate(Module &M, const PreservedAnalyses &PA,
                   ModuleAnalysisManager::Invalidator &Inv);
+
+  using iterator = MapT::iterator;
+  using const_iterator = MapT::const_iterator;
+  using reverse_iterator = MapT::reverse_iterator;
+  using const_reverse_iterator = MapT::const_reverse_iterator;
+
+  iterator begin() { return Strategies.begin(); }
+  const_iterator begin() const { return Strategies.begin(); }
+  iterator end() { return Strategies.end(); }
+  const_iterator end() const { return Strategies.end(); }
+
+  reverse_iterator rbegin() { return Strategies.rbegin(); }
+  const_reverse_iterator rbegin() const { return Strategies.rbegin(); }
+  reverse_iterator rend() { return Strategies.rend(); }
+  const_reverse_iterator rend() const { return Strategies.rend(); }
+
+  bool empty() const { return Strategies.empty(); }
+
+  GCStrategy &operator[](StringRef GCName) { return *Strategies[GCName]; }
+
+  std::pair<iterator, bool> try_emplace(StringRef GCName) {
+    return Strategies.try_emplace(GCName);
+  }
+
+  bool contains(StringRef GCName) const {
+    return Strategies.find(GCName) != Strategies.end();
+  }
 };
 
 /// An analysis pass which caches information about the entire Module.
diff --git a/llvm/include/llvm/IR/GCStrategy.h b/llvm/include/llvm/IR/GCStrategy.h
index cbfbe23aaa068..6b813554d6544 100644
--- a/llvm/include/llvm/IR/GCStrategy.h
+++ b/llvm/include/llvm/IR/GCStrategy.h
@@ -63,6 +63,7 @@ class Type;
 class GCStrategy {
 private:
   friend class GCModuleInfo;
+  friend class CollectorMetadataAnalysis;
 
   std::string Name;
 
diff --git a/llvm/lib/CodeGen/GCMetadata.cpp b/llvm/lib/CodeGen/GCMetadata.cpp
index fa87b14e708e1..fe3a15b982074 100644
--- a/llvm/lib/CodeGen/GCMetadata.cpp
+++ b/llvm/lib/CodeGen/GCMetadata.cpp
@@ -26,7 +26,7 @@ bool GCStrategyMap::invalidate(Module &M, const PreservedAnalyses &PA,
   for (const auto &F : M) {
     if (F.isDeclaration() || !F.hasGC())
       continue;
-    if (!StrategyMap.contains(F.getGC()))
+    if (!contains(F.getGC()))
       return true;
   }
   return false;
@@ -36,17 +36,18 @@ AnalysisKey CollectorMetadataAnalysis::Key;
 
 CollectorMetadataAnalysis::Result
 CollectorMetadataAnalysis::run(Module &M, ModuleAnalysisManager &MAM) {
-  Result R;
-  auto &Map = R.StrategyMap;
+  Result StrategyMap;
   for (auto &F : M) {
     if (F.isDeclaration() || !F.hasGC())
       continue;
-    auto GCName = F.getGC();
-    auto [It, Inserted] = Map.try_emplace(GCName);
-    if (Inserted)
+    StringRef GCName = F.getGC();
+    auto [It, Inserted] = StrategyMap.try_emplace(GCName);
+    if (Inserted) {
       It->second = getGCStrategy(GCName);
+      It->second->Name = GCName;
+    }
   }
-  return R;
+  return StrategyMap;
 }
 
 AnalysisKey GCFunctionAnalysis::Key;
@@ -61,9 +62,8 @@ GCFunctionAnalysis::run(Function &F, FunctionAnalysisManager &FAM) {
       MAMProxy.cachedResultExists<CollectorMetadataAnalysis>(*F.getParent()) &&
       "This pass need module analysis `collector-metadata`!");
   auto &Map =
-      MAMProxy.getCachedResult<CollectorMetadataAnalysis>(*F.getParent())
-          ->StrategyMap;
-  GCFunctionInfo Info(F, *Map[F.getGC()]);
+      *MAMProxy.getCachedResult<CollectorMetadataAnalysis>(*F.getParent());
+  GCFunctionInfo Info(F, Map[F.getGC()]);
   return Info;
 }
 
diff --git a/llvm/lib/CodeGen/ShadowStackGCLowering.cpp b/llvm/lib/CodeGen/ShadowStackGCLowering.cpp
index 60c8372577a93..1f9beb84ef62d 100644
--- a/llvm/lib/CodeGen/ShadowStackGCLowering.cpp
+++ b/llvm/lib/CodeGen/ShadowStackGCLowering.cpp
@@ -109,7 +109,7 @@ class ShadowStackGCLowering : public FunctionPass {
 PreservedAnalyses ShadowStackGCLoweringPass::run(Module &M,
                                                  ModuleAnalysisManager &MAM) {
   auto &Map = MAM.getResult<CollectorMetadataAnalysis>(M);
-  if (Map.StrategyMap.contains("shadow-stack"))
+  if (!Map.contains("shadow-stack"))
     return PreservedAnalyses::all();
 
   ShadowStackGCLoweringImpl Impl;
diff --git a/llvm/unittests/CodeGen/CMakeLists.txt b/llvm/unittests/CodeGen/CMakeLists.txt
index 4f580e7539f4d..cc8ccc1480018 100644
--- a/llvm/unittests/CodeGen/CMakeLists.txt
+++ b/llvm/unittests/CodeGen/CMakeLists.txt
@@ -29,6 +29,7 @@ add_llvm_unittest(CodeGenTests
   DIETest.cpp
   DroppedVariableStatsMIRTest.cpp
   DwarfStringPoolEntryRefTest.cpp
+  GCMetadata.cpp
   InstrRefLDVTest.cpp
   LowLevelTypeTest.cpp
   LexicalScopesTest.cpp
diff --git a/llvm/unittests/CodeGen/GCMetadata.cpp b/llvm/unittests/CodeGen/GCMetadata.cpp
new file mode 100644
index 0000000000000..320145ae0d989
--- /dev/null
+++ b/llvm/unittests/CodeGen/GCMetadata.cpp
@@ -0,0 +1,84 @@
+//===- llvm/unittest/CodeGen/PassManager.cpp - PassManager tests ----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+// Test that the various MachineFunction pass managers, adaptors, analyses, and
+// analysis managers work.
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/GCMetadata.h"
+#include "llvm/Analysis/CGSCCPassManager.h"
+#include "llvm/Analysis/LoopAnalysisManager.h"
+#include "llvm/AsmParser/Parser.h"
+#include "llvm/IR/Analysis.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/MC/TargetRegistry.h"
+#include "llvm/Passes/PassBuilder.h"
+#include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/TargetSelect.h"
+#include "llvm/Target/TargetMachine.h"
+#include "llvm/TargetParser/Host.h"
+#include "llvm/TargetParser/Triple.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+std::unique_ptr<Module> parseIR(LLVMContext &Context, const char *IR) {
+  SMDiagnostic Err;
+  return parseAssemblyString(IR, Err, Context);
+}
+
+class GCMetadataTest : public ::testing::Test {
+protected:
+  LLVMContext Context;
+  std::unique_ptr<Module> M;
+
+public:
+  GCMetadataTest()
+      : M(parseIR(Context, R"(
+%Env = type ptr
+
+define void @.main(%Env) gc "shadow-stack" {
+	%Root = alloca %Env
+	call void @llvm.gcroot( ptr %Root, %Env null )
+	unreachable
+}
+
+define void @g() gc "erlang" {
+entry:
+	ret void
+}
+
+declare void @llvm.gcroot(ptr, %Env)
+)")) {}
+};
+
+TEST_F(GCMetadataTest, Basic) {
+  LoopAnalysisManager LAM;
+  FunctionAnalysisManager FAM;
+  CGSCCAnalysisManager CGAM;
+  ModuleAnalysisManager MAM;
+  PassBuilder PB;
+  PB.registerModuleAnalyses(MAM);
+  PB.registerCGSCCAnalyses(CGAM);
+  PB.registerFunctionAnalyses(FAM);
+  PB.registerLoopAnalyses(LAM);
+  PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
+
+  ModulePassManager MPM;
+  FunctionPassManager FPM;
+  GCStrategyMap &StrategyMap = MAM.getResult<CollectorMetadataAnalysis>(*M);
+  for (auto &[GCName, Strategy] : StrategyMap)
+    EXPECT_EQ(GCName, Strategy->getName());
+  for (auto &[GCName, Strategy] : llvm::reverse(StrategyMap))
+    EXPECT_EQ(GCName, Strategy->getName());
+}
+
+} // namespace

@paperchalice paperchalice merged commit 1483659 into llvm:main May 14, 2025
11 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented May 14, 2025

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2915

Here is the relevant piece of the build log for the reference
Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/90/95' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-24176-90-95.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=95 GTEST_SHARD_INDEX=90 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************


@paperchalice paperchalice deleted the gc-strategy branch May 15, 2025 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.