[KeyInstr] Hide new MDNodeKeyImpl<DILocation> fields #138296

OCHyams · 2025-05-02T15:32:57Z

Follow up to #133477.

This prevents a compile time regression pointed out by nikic
https://llvm-compile-time-tracker.com/compare.php?from=d33c6764680ed78ffe824a83b6a33c0b609bafce&to=0c7c82af230bd525bb96a54ca9480797b5fa2a42&stat=instructions:u

The additional checks in the methods seem to cause most of the regression
(rather than being a consequence of increased size due to the fields
themselves).

The additional ifdefs are somewhat ugly - but I'm not sure if we can get around
that for now?

llvmbot · 2025-05-02T15:33:32Z

@llvm/pr-subscribers-llvm-ir

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Please ignore the first commit, which is in review elsewhere.

(compile-time-tracker confirmed the fix, but I since force pushed the branch and lost the results - I'll regenerate them and add if needed, but wanted to get the PR up regardless)

Follow up to #133477.

This prevents a compile time regression pointed out by @nikic
https://llvm-compile-time-tracker.com/compare.php?from=d33c6764680ed78ffe824a83b6a33c0b609bafce&to=0c7c82af230bd525bb96a54ca9480797b5fa2a42&stat=instructions:u

The additional checks in the methods seem to cause most of the regression
(rather than being a consequence of increased size due to the fields
themselves).

The additional ifdefs are somewhat ugly - but I'm not sure if we can get around
that for now?

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

2 Files Affected:

(modified) llvm/include/llvm/IR/DebugInfoMetadata.h (+1-1)
(modified) llvm/lib/IR/LLVMContextImpl.h (+31-8)

diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 91cda271f498b..0f6a206cab75f 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2237,7 +2237,7 @@ class DILocation : public MDNode {
   friend class MDNode;
 #ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
   uint64_t AtomGroup : 61;
-  uint8_t AtomRank : 3;
+  uint64_t AtomRank : 3;
 #endif
 
   DILocation(LLVMContext &C, StorageType Storage, unsigned Line,
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index ad5fb802029fe..5c2b5cd3a19cc 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -315,30 +315,53 @@ template <> struct MDNodeKeyImpl<DILocation> {
   Metadata *Scope;
   Metadata *InlinedAt;
   bool ImplicitCode;
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
   uint64_t AtomGroup : 61;
-  uint8_t AtomRank : 3;
+  uint64_t AtomRank : 3;
+#endif
 
   MDNodeKeyImpl(unsigned Line, unsigned Column, Metadata *Scope,
                 Metadata *InlinedAt, bool ImplicitCode, uint64_t AtomGroup,
                 uint8_t AtomRank)
       : Line(Line), Column(Column), Scope(Scope), InlinedAt(InlinedAt),
-        ImplicitCode(ImplicitCode), AtomGroup(AtomGroup), AtomRank(AtomRank) {}
+        ImplicitCode(ImplicitCode)
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+        ,
+        AtomGroup(AtomGroup), AtomRank(AtomRank)
+#endif
+  {
+  }
 
   MDNodeKeyImpl(const DILocation *L)
       : Line(L->getLine()), Column(L->getColumn()), Scope(L->getRawScope()),
-        InlinedAt(L->getRawInlinedAt()), ImplicitCode(L->isImplicitCode()),
-        AtomGroup(L->getAtomGroup()), AtomRank(L->getAtomRank()) {}
+        InlinedAt(L->getRawInlinedAt()), ImplicitCode(L->isImplicitCode())
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+        ,
+        AtomGroup(L->getAtomGroup()), AtomRank(L->getAtomRank())
+#endif
+  {
+  }
 
   bool isKeyOf(const DILocation *RHS) const {
     return Line == RHS->getLine() && Column == RHS->getColumn() &&
            Scope == RHS->getRawScope() && InlinedAt == RHS->getRawInlinedAt() &&
-           ImplicitCode == RHS->isImplicitCode() &&
-           AtomGroup == RHS->getAtomGroup() && AtomRank == RHS->getAtomRank();
+           ImplicitCode == RHS->isImplicitCode()
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+           && AtomGroup == RHS->getAtomGroup() &&
+           AtomRank == RHS->getAtomRank();
+#else
+        ;
+#endif
   }
 
   unsigned getHashValue() const {
-    return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode, AtomGroup,
-                        AtomRank);
+    return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+                        ,
+                        AtomGroup, (uint8_t)AtomRank);
+#else
+    );
+#endif
   }
 };

llvmbot · 2025-05-02T15:33:33Z

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Please ignore the first commit, which is in review elsewhere.

(compile-time-tracker confirmed the fix, but I since force pushed the branch and lost the results - I'll regenerate them and add if needed, but wanted to get the PR up regardless)

Follow up to #133477.

This prevents a compile time regression pointed out by @nikic
https://llvm-compile-time-tracker.com/compare.php?from=d33c6764680ed78ffe824a83b6a33c0b609bafce&to=0c7c82af230bd525bb96a54ca9480797b5fa2a42&stat=instructions:u

The additional checks in the methods seem to cause most of the regression
(rather than being a consequence of increased size due to the fields
themselves).

The additional ifdefs are somewhat ugly - but I'm not sure if we can get around
that for now?

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

2 Files Affected:

(modified) llvm/include/llvm/IR/DebugInfoMetadata.h (+1-1)
(modified) llvm/lib/IR/LLVMContextImpl.h (+31-8)

diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h
index 91cda271f498b..0f6a206cab75f 100644
--- a/llvm/include/llvm/IR/DebugInfoMetadata.h
+++ b/llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2237,7 +2237,7 @@ class DILocation : public MDNode {
   friend class MDNode;
 #ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
   uint64_t AtomGroup : 61;
-  uint8_t AtomRank : 3;
+  uint64_t AtomRank : 3;
 #endif
 
   DILocation(LLVMContext &C, StorageType Storage, unsigned Line,
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index ad5fb802029fe..5c2b5cd3a19cc 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -315,30 +315,53 @@ template <> struct MDNodeKeyImpl<DILocation> {
   Metadata *Scope;
   Metadata *InlinedAt;
   bool ImplicitCode;
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
   uint64_t AtomGroup : 61;
-  uint8_t AtomRank : 3;
+  uint64_t AtomRank : 3;
+#endif
 
   MDNodeKeyImpl(unsigned Line, unsigned Column, Metadata *Scope,
                 Metadata *InlinedAt, bool ImplicitCode, uint64_t AtomGroup,
                 uint8_t AtomRank)
       : Line(Line), Column(Column), Scope(Scope), InlinedAt(InlinedAt),
-        ImplicitCode(ImplicitCode), AtomGroup(AtomGroup), AtomRank(AtomRank) {}
+        ImplicitCode(ImplicitCode)
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+        ,
+        AtomGroup(AtomGroup), AtomRank(AtomRank)
+#endif
+  {
+  }
 
   MDNodeKeyImpl(const DILocation *L)
       : Line(L->getLine()), Column(L->getColumn()), Scope(L->getRawScope()),
-        InlinedAt(L->getRawInlinedAt()), ImplicitCode(L->isImplicitCode()),
-        AtomGroup(L->getAtomGroup()), AtomRank(L->getAtomRank()) {}
+        InlinedAt(L->getRawInlinedAt()), ImplicitCode(L->isImplicitCode())
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+        ,
+        AtomGroup(L->getAtomGroup()), AtomRank(L->getAtomRank())
+#endif
+  {
+  }
 
   bool isKeyOf(const DILocation *RHS) const {
     return Line == RHS->getLine() && Column == RHS->getColumn() &&
            Scope == RHS->getRawScope() && InlinedAt == RHS->getRawInlinedAt() &&
-           ImplicitCode == RHS->isImplicitCode() &&
-           AtomGroup == RHS->getAtomGroup() && AtomRank == RHS->getAtomRank();
+           ImplicitCode == RHS->isImplicitCode()
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+           && AtomGroup == RHS->getAtomGroup() &&
+           AtomRank == RHS->getAtomRank();
+#else
+        ;
+#endif
   }
 
   unsigned getHashValue() const {
-    return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode, AtomGroup,
-                        AtomRank);
+    return hash_combine(Line, Column, Scope, InlinedAt, ImplicitCode
+#ifdef EXPERIMENTAL_KEY_INSTRUCTIONS
+                        ,
+                        AtomGroup, (uint8_t)AtomRank);
+#else
+    );
+#endif
   }
 };

pogo59 · 2025-05-02T15:56:00Z

Are you expecting the perf drop to go away once KI is finished?

nikic · 2025-05-02T15:59:39Z

Are you expecting the perf drop to go away once KI is finished?

It's expected to go away / be greatly mitigated once https://discourse.llvm.org/t/rfc-refactoring-dilocation-to-use-compact-function-local-storage/85618 is implemented.

@nikic

Follow up to llvm#133477. This prevents a compile time regression pointed out by @nikic https://llvm-compile-time-tracker.com/compare.php?from=d33c6764680ed78ffe824a83b6a33c0b609bafce&to=0c7c82af230bd525bb96a54ca9480797b5fa2a42&stat=instructions:u The additional checks in the methods seem to cause most of the regression (rather than being a consequence of increased size due to the fields themselves). The additional ifdefs are somewhat ugly - but I'm not sure if we can get around that for now?

OCHyams · 2025-05-06T08:09:38Z

Are you expecting the perf drop to go away once KI is finished?

Hi :) As nikic says - we're going to mitigate it

Follow up to llvm#133477. This prevents a compile time regression pointed out by nikic The additional checks in the methods seem to cause most of the regression (rather than being a consequence of increased size due to the fields themselves). The additional ifdefs are somewhat ugly, but will eventually be removed.

OCHyams requested review from nikic and jmorse May 2, 2025 15:32

llvmbot added debuginfo llvm:ir labels May 2, 2025

nikic approved these changes May 2, 2025

View reviewed changes

OCHyams force-pushed the ki-hide-more-stuff branch from 24787fb to 86a0549 Compare May 6, 2025 08:06

OCHyams merged commit 6d85de8 into llvm:main May 6, 2025
6 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[KeyInstr] Hide new MDNodeKeyImpl<DILocation> fields #138296

[KeyInstr] Hide new MDNodeKeyImpl<DILocation> fields #138296

Uh oh!

OCHyams commented May 2, 2025 •

edited

Loading

Uh oh!

llvmbot commented May 2, 2025

Uh oh!

llvmbot commented May 2, 2025

Uh oh!

pogo59 commented May 2, 2025

Uh oh!

nikic commented May 2, 2025

Uh oh!

OCHyams commented May 6, 2025

Uh oh!

Uh oh!

Uh oh!

[KeyInstr] Hide new MDNodeKeyImpl<DILocation> fields #138296

[KeyInstr] Hide new MDNodeKeyImpl<DILocation> fields #138296

Uh oh!

Conversation

OCHyams commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 2, 2025

Uh oh!

llvmbot commented May 2, 2025

Uh oh!

pogo59 commented May 2, 2025

Uh oh!

nikic commented May 2, 2025

Uh oh!

OCHyams commented May 6, 2025

Uh oh!

Uh oh!

Uh oh!

OCHyams commented May 2, 2025 •

edited

Loading