[HLSL] Refactoring DXILABI.h to not depend on scope printer #153840

joaosaffran · 2025-08-15T17:27:19Z

This patch refactors DXILABI to remove the dependency on scope printer.
Closes: #153827

llvmbot · 2025-08-15T17:27:50Z

@llvm/pr-subscribers-hlsl

@llvm/pr-subscribers-llvm-support

Author: None (joaosaffran)

Changes

This patch refactors DXILABI to remove the dependency on scope printer.
Closes: #153827

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

4 Files Affected:

(modified) llvm/include/llvm/Support/DXILABI.h (+1-3)
(modified) llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp (+2-3)
(modified) llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp (+2-6)
(modified) llvm/lib/Support/DXILABI.cpp (+5-1)

diff --git a/llvm/include/llvm/Support/DXILABI.h b/llvm/include/llvm/Support/DXILABI.h
index 2dcdd73415be2..47b3a6619344f 100644
--- a/llvm/include/llvm/Support/DXILABI.h
+++ b/llvm/include/llvm/Support/DXILABI.h
@@ -18,7 +18,6 @@
 #define LLVM_SUPPORT_DXILABI_H
 
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/ScopedPrinter.h"
 #include <cstdint>
 
 namespace llvm {
@@ -101,9 +100,8 @@ enum class SamplerFeedbackType : uint32_t {
 const unsigned MinWaveSize = 4;
 const unsigned MaxWaveSize = 128;
 
-LLVM_ABI ArrayRef<EnumEntry<ResourceClass>> getResourceClasses();
-
 LLVM_ABI StringRef getResourceClassName(ResourceClass RC);
+LLVM_ABI StringRef getResourceClassName(uint8_t RC);
 
 } // namespace dxil
 } // namespace llvm
diff --git a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
index 050cc46e8c9b0..30b597dbad497 100644
--- a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
+++ b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Frontend/HLSL/HLSLRootSignature.h"
+#include "llvm/Support/DXILABI.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 namespace llvm {
@@ -93,9 +94,7 @@ static raw_ostream &operator<<(raw_ostream &OS,
 }
 
 static raw_ostream &operator<<(raw_ostream &OS, const ClauseType &Type) {
-  OS << enumToStringRef(dxil::ResourceClass(llvm::to_underlying(Type)),
-                        dxil::getResourceClasses());
-
+  OS << dxil::getResourceClassName(llvm::to_underlying(Type));
   return OS;
 }
 
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 157bfc665b207..c24475ed1bcf6 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -119,9 +119,7 @@ MDNode *MetadataBuilder::BuildRootConstants(const RootConstants &Constants) {
 
 MDNode *MetadataBuilder::BuildRootDescriptor(const RootDescriptor &Descriptor) {
   IRBuilder<> Builder(Ctx);
-  StringRef ResName =
-      enumToStringRef(dxil::ResourceClass(to_underlying(Descriptor.Type)),
-                      dxil::getResourceClasses());
+  StringRef ResName = dxil::getResourceClassName(to_underlying(Descriptor.Type));
   assert(!ResName.empty() && "Provided an invalid Resource Class");
   SmallString<7> Name({"Root", ResName});
   Metadata *Operands[] = {
@@ -161,9 +159,7 @@ MDNode *MetadataBuilder::BuildDescriptorTable(const DescriptorTable &Table) {
 MDNode *MetadataBuilder::BuildDescriptorTableClause(
     const DescriptorTableClause &Clause) {
   IRBuilder<> Builder(Ctx);
-  StringRef ResName =
-      enumToStringRef(dxil::ResourceClass(to_underlying(Clause.Type)),
-                      dxil::getResourceClasses());
+  StringRef ResName = dxil::getResourceClassName(to_underlying(Clause.Type));
   assert(!ResName.empty() && "Provided an invalid Resource Class");
   Metadata *Operands[] = {
       MDString::get(Ctx, ResName),
diff --git a/llvm/lib/Support/DXILABI.cpp b/llvm/lib/Support/DXILABI.cpp
index 261fe1ef98278..5b7c0669881ea 100644
--- a/llvm/lib/Support/DXILABI.cpp
+++ b/llvm/lib/Support/DXILABI.cpp
@@ -25,10 +25,14 @@ static const EnumEntry<dxil::ResourceClass> ResourceClassNames[] = {
     {"Sampler", llvm::dxil::ResourceClass::Sampler},
 };
 
-ArrayRef<EnumEntry<llvm::dxil::ResourceClass>> dxil::getResourceClasses() {
+ArrayRef<EnumEntry<llvm::dxil::ResourceClass>> getResourceClasses() {
   return ArrayRef(ResourceClassNames);
 }
 
 StringRef dxil::getResourceClassName(dxil::ResourceClass RC) {
   return enumToStringRef(RC, getResourceClasses());
 }
+
+StringRef dxil::getResourceClassName(uint8_t RC) {
+  return enumToStringRef(ResourceClass(RC), getResourceClasses());
+}

llvmbot · 2025-08-15T17:27:50Z

@llvm/pr-subscribers-backend-directx

Author: None (joaosaffran)

Changes

This patch refactors DXILABI to remove the dependency on scope printer.
Closes: #153827

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

4 Files Affected:

(modified) llvm/include/llvm/Support/DXILABI.h (+1-3)
(modified) llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp (+2-3)
(modified) llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp (+2-6)
(modified) llvm/lib/Support/DXILABI.cpp (+5-1)

diff --git a/llvm/include/llvm/Support/DXILABI.h b/llvm/include/llvm/Support/DXILABI.h
index 2dcdd73415be2..47b3a6619344f 100644
--- a/llvm/include/llvm/Support/DXILABI.h
+++ b/llvm/include/llvm/Support/DXILABI.h
@@ -18,7 +18,6 @@
 #define LLVM_SUPPORT_DXILABI_H
 
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/ScopedPrinter.h"
 #include <cstdint>
 
 namespace llvm {
@@ -101,9 +100,8 @@ enum class SamplerFeedbackType : uint32_t {
 const unsigned MinWaveSize = 4;
 const unsigned MaxWaveSize = 128;
 
-LLVM_ABI ArrayRef<EnumEntry<ResourceClass>> getResourceClasses();
-
 LLVM_ABI StringRef getResourceClassName(ResourceClass RC);
+LLVM_ABI StringRef getResourceClassName(uint8_t RC);
 
 } // namespace dxil
 } // namespace llvm
diff --git a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
index 050cc46e8c9b0..30b597dbad497 100644
--- a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
+++ b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Frontend/HLSL/HLSLRootSignature.h"
+#include "llvm/Support/DXILABI.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 namespace llvm {
@@ -93,9 +94,7 @@ static raw_ostream &operator<<(raw_ostream &OS,
 }
 
 static raw_ostream &operator<<(raw_ostream &OS, const ClauseType &Type) {
-  OS << enumToStringRef(dxil::ResourceClass(llvm::to_underlying(Type)),
-                        dxil::getResourceClasses());
-
+  OS << dxil::getResourceClassName(llvm::to_underlying(Type));
   return OS;
 }
 
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 157bfc665b207..c24475ed1bcf6 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -119,9 +119,7 @@ MDNode *MetadataBuilder::BuildRootConstants(const RootConstants &Constants) {
 
 MDNode *MetadataBuilder::BuildRootDescriptor(const RootDescriptor &Descriptor) {
   IRBuilder<> Builder(Ctx);
-  StringRef ResName =
-      enumToStringRef(dxil::ResourceClass(to_underlying(Descriptor.Type)),
-                      dxil::getResourceClasses());
+  StringRef ResName = dxil::getResourceClassName(to_underlying(Descriptor.Type));
   assert(!ResName.empty() && "Provided an invalid Resource Class");
   SmallString<7> Name({"Root", ResName});
   Metadata *Operands[] = {
@@ -161,9 +159,7 @@ MDNode *MetadataBuilder::BuildDescriptorTable(const DescriptorTable &Table) {
 MDNode *MetadataBuilder::BuildDescriptorTableClause(
     const DescriptorTableClause &Clause) {
   IRBuilder<> Builder(Ctx);
-  StringRef ResName =
-      enumToStringRef(dxil::ResourceClass(to_underlying(Clause.Type)),
-                      dxil::getResourceClasses());
+  StringRef ResName = dxil::getResourceClassName(to_underlying(Clause.Type));
   assert(!ResName.empty() && "Provided an invalid Resource Class");
   Metadata *Operands[] = {
       MDString::get(Ctx, ResName),
diff --git a/llvm/lib/Support/DXILABI.cpp b/llvm/lib/Support/DXILABI.cpp
index 261fe1ef98278..5b7c0669881ea 100644
--- a/llvm/lib/Support/DXILABI.cpp
+++ b/llvm/lib/Support/DXILABI.cpp
@@ -25,10 +25,14 @@ static const EnumEntry<dxil::ResourceClass> ResourceClassNames[] = {
     {"Sampler", llvm::dxil::ResourceClass::Sampler},
 };
 
-ArrayRef<EnumEntry<llvm::dxil::ResourceClass>> dxil::getResourceClasses() {
+ArrayRef<EnumEntry<llvm::dxil::ResourceClass>> getResourceClasses() {
   return ArrayRef(ResourceClassNames);
 }
 
 StringRef dxil::getResourceClassName(dxil::ResourceClass RC) {
   return enumToStringRef(RC, getResourceClasses());
 }
+
+StringRef dxil::getResourceClassName(uint8_t RC) {
+  return enumToStringRef(ResourceClass(RC), getResourceClasses());
+}

github-actions · 2025-08-15T17:29:03Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

github-actions · 2025-08-15T17:30:15Z

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

bogner · 2025-08-15T17:34:31Z

llvm/include/llvm/Support/DXILABI.h

 LLVM_ABI StringRef getResourceClassName(ResourceClass RC);
+LLVM_ABI StringRef getResourceClassName(uint8_t RC);


This is not a good API to add - why would we want anything other than the strongly typed version of this function?

bogner · 2025-08-15T17:37:11Z

llvm/lib/Support/DXILABI.cpp

 StringRef dxil::getResourceClassName(dxil::ResourceClass RC) {
  return enumToStringRef(RC, getResourceClasses());
 }


Using ScopedPrinter for this seems like overkill if we aren't using the EnumEntry thing for anything else. Would it be better to just implement dxil::getResourceClassName as a simple switch statement?

llvm-beanz

The use of to_underlying in this PR (and other changes) is deeply wrong.

In this PR your to_underlying calls are basically doing enum->int->enum conversions.

Please look critically at all the places you're using to_underlying to ensure the conversions make sense and aren't just shedding type safety for no discernable benefit.

llvm-beanz

We should not merge this without fixing the to_underlying conversions.

damyanp · 2025-08-15T21:35:36Z

@llvm-beanz - this is in response to #152229 (comment).

Since this change is moving around existing to_underlying things rather than introducing new ones, woudl it be ok to get this in (thus fixing the 2% build time increase) and then prioritize landing #151032 (and making sure that the cases in this PR are addressed in that one as well)?

@joaosaffran - it would be good if the PR description more directly explained the motivation for this change without having to read through issues and links to comments on PRs.

damyanp · 2025-08-15T21:43:29Z

Another alternative would be to fix the (I think) 3 cases of misused to_underlying that are modified in this PR, and subsequently fix all the others.

llvm-beanz · 2025-08-15T21:58:48Z

Another alternative would be to fix the (I think) 3 cases of misused to_underlying that are modified in this PR, and subsequently fix all the others.

This is what I intended in my comment. We should not be changing lines of code from one wrong state to another wrong state. The use of to_underlying in the modified lines here is clearly wrong, so we should just fix it.

llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

joaosaffran · 2025-08-15T22:02:33Z

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

@@ -161,9 +160,8 @@ MDNode *MetadataBuilder::BuildDescriptorTable(const DescriptorTable &Table) {
 MDNode *MetadataBuilder::BuildDescriptorTableClause(
    const DescriptorTableClause &Clause) {
  IRBuilder<> Builder(Ctx);
-  StringRef ResName =


Not the same enum @llvm-beanz

llvm-project/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h

Line 85 in b7d6f48

using ClauseType = llvm::dxil::ResourceClass;

llvm-beanz · 2025-08-15T22:16:16Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo. Please turn off Keep my email addresses private setting in your account. See LLVM Developer Policy and LLVM Discourse for more information.

@joaosaffran you should fix this

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp

llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp

nikic · 2025-08-16T07:30:24Z

llvm/include/llvm/Support/DXILABI.h

@@ -18,7 +18,6 @@
 #define LLVM_SUPPORT_DXILABI_H

 #include "llvm/ADT/StringRef.h"


Can also replace this StringRef.h include with a forward declaration.

Joao Saffran added 2 commits August 15, 2025 10:12

refactoring

f2d933e

clean up

9073f07

llvmbot added backend:DirectX HLSL HLSL Language Support llvm:support labels Aug 15, 2025

bogner reviewed Aug 15, 2025

View reviewed changes

removing API and removing enumToStringRef

1f4332c

bogner approved these changes Aug 15, 2025

View reviewed changes

inbelic approved these changes Aug 15, 2025

View reviewed changes

bob80905 approved these changes Aug 15, 2025

View reviewed changes

llvm-beanz reviewed Aug 15, 2025

View reviewed changes

llvm-beanz requested changes Aug 15, 2025

View reviewed changes

joaosaffran commented Aug 15, 2025

View reviewed changes

Joao Saffran added 2 commits August 15, 2025 15:12

fix

be68476

refactor

02a7c17

llvm-beanz reviewed Aug 15, 2025

View reviewed changes

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp Show resolved Hide resolved

llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp Outdated Show resolved Hide resolved

llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp Outdated Show resolved Hide resolved

fix

7b84bca

llvm-beanz approved these changes Aug 15, 2025

View reviewed changes

damyanp approved these changes Aug 16, 2025

View reviewed changes

joaosaffran merged commit 37729d8 into llvm:main Aug 16, 2025
10 checks passed

nikic reviewed Aug 16, 2025

View reviewed changes

		LLVM_ABI StringRef getResourceClassName(ResourceClass RC);
		LLVM_ABI StringRef getResourceClassName(uint8_t RC);

		@@ -18,7 +18,6 @@
		#define LLVM_SUPPORT_DXILABI_H

		#include "llvm/ADT/StringRef.h"

[HLSL] Refactoring DXILABI.h to not depend on scope printer #153840

[HLSL] Refactoring DXILABI.h to not depend on scope printer #153840

Conversation

joaosaffran commented Aug 15, 2025

Uh oh!

llvmbot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 15, 2025

Uh oh!

github-actions bot commented Aug 15, 2025

Uh oh!

github-actions bot commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bogner Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

bogner Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

damyanp commented Aug 15, 2025

Uh oh!

damyanp commented Aug 15, 2025

Uh oh!

llvm-beanz commented Aug 15, 2025

Uh oh!

Uh oh!

Uh oh!

joaosaffran Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

llvm-beanz Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

llvm-beanz commented Aug 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Aug 15, 2025 •

edited

Loading

github-actions bot commented Aug 15, 2025 •

edited

Loading