[NFC][HLSL] Replace uses of getResourceName/printEnum
#152211
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Introduce the `enumToStringRef` enum into `ScopedPrinter.h` that replicates `enumToString` behaviour, expect that instead of returning a hex value string, it just returns an empty string. This allows us to return a StringRef and easily check if an invalid enum was provided based on the StringRef size This then uses `enumToStringRef` to remove the redundant `getResourceName` and `printEnum` functions. Resolves:
Member
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-hlsl Author: Finn Plummer (inbelic) ChangesIntroduce the This then uses Resolves: #151200. Full diff: https://github.com/llvm/llvm-project/pull/152211.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Support/ScopedPrinter.h b/llvm/include/llvm/Support/ScopedPrinter.h
index e6c4cc4a4ea13..a25658c964c25 100644
--- a/llvm/include/llvm/Support/ScopedPrinter.h
+++ b/llvm/include/llvm/Support/ScopedPrinter.h
@@ -107,6 +107,14 @@ std::string enumToString(T Value, ArrayRef<EnumEntry<TEnum>> EnumValues) {
return utohexstr(Value, true);
}
+template <typename T, typename TEnum>
+StringRef enumToStringRef(T Value, ArrayRef<EnumEntry<TEnum>> EnumValues) {
+ for (const EnumEntry<TEnum> &EnumItem : EnumValues)
+ if (EnumItem.Value == Value)
+ return EnumItem.AltName;
+ return "";
+}
+
class LLVM_ABI ScopedPrinter {
public:
enum class ScopedPrinterKind {
diff --git a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
index 78c20a6c5c9ff..22d44e98feffe 100644
--- a/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
+++ b/llvm/lib/Frontend/HLSL/HLSLRootSignature.cpp
@@ -17,24 +17,6 @@ namespace llvm {
namespace hlsl {
namespace rootsig {
-template <typename T>
-static std::optional<StringRef> getEnumName(const T Value,
- ArrayRef<EnumEntry<T>> Enums) {
- for (const auto &EnumItem : Enums)
- if (EnumItem.Value == Value)
- return EnumItem.Name;
- return std::nullopt;
-}
-
-template <typename T>
-static raw_ostream &printEnum(raw_ostream &OS, const T Value,
- ArrayRef<EnumEntry<T>> Enums) {
- auto MaybeName = getEnumName(Value, Enums);
- if (MaybeName)
- OS << *MaybeName;
- return OS;
-}
-
template <typename T>
static raw_ostream &printFlags(raw_ostream &OS, const T Value,
ArrayRef<EnumEntry<T>> Flags) {
@@ -46,9 +28,9 @@ static raw_ostream &printFlags(raw_ostream &OS, const T Value,
if (FlagSet)
OS << " | ";
- auto MaybeFlag = getEnumName(T(Bit), Flags);
- if (MaybeFlag)
- OS << *MaybeFlag;
+ StringRef MaybeFlag = enumToStringRef(T(Bit), Flags);
+ if (0 < MaybeFlag.size())
+ OS << MaybeFlag;
else
OS << "invalid: " << Bit;
@@ -70,43 +52,42 @@ static const EnumEntry<RegisterType> RegisterNames[] = {
};
static raw_ostream &operator<<(raw_ostream &OS, const Register &Reg) {
- printEnum(OS, Reg.ViewType, ArrayRef(RegisterNames));
- OS << Reg.Number;
+ OS << enumToStringRef(Reg.ViewType, ArrayRef(RegisterNames)) << Reg.Number;
return OS;
}
static raw_ostream &operator<<(raw_ostream &OS,
const llvm::dxbc::ShaderVisibility &Visibility) {
- printEnum(OS, Visibility, dxbc::getShaderVisibility());
+ OS << enumToStringRef(Visibility, dxbc::getShaderVisibility());
return OS;
}
static raw_ostream &operator<<(raw_ostream &OS,
const llvm::dxbc::SamplerFilter &Filter) {
- printEnum(OS, Filter, dxbc::getSamplerFilters());
+ OS << enumToStringRef(Filter, dxbc::getSamplerFilters());
return OS;
}
static raw_ostream &operator<<(raw_ostream &OS,
const dxbc::TextureAddressMode &Address) {
- printEnum(OS, Address, dxbc::getTextureAddressModes());
+ OS << enumToStringRef(Address, dxbc::getTextureAddressModes());
return OS;
}
static raw_ostream &operator<<(raw_ostream &OS,
const dxbc::ComparisonFunc &CompFunc) {
- printEnum(OS, CompFunc, dxbc::getComparisonFuncs());
+ OS << enumToStringRef(CompFunc, dxbc::getComparisonFuncs());
return OS;
}
static raw_ostream &operator<<(raw_ostream &OS,
const dxbc::StaticBorderColor &BorderColor) {
- printEnum(OS, BorderColor, dxbc::getStaticBorderColors());
+ OS << enumToStringRef(BorderColor, dxbc::getStaticBorderColors());
return OS;
}
@@ -119,8 +100,8 @@ static const EnumEntry<dxil::ResourceClass> ResourceClassNames[] = {
};
static raw_ostream &operator<<(raw_ostream &OS, const ClauseType &Type) {
- printEnum(OS, dxil::ResourceClass(llvm::to_underlying(Type)),
- ArrayRef(ResourceClassNames));
+ OS << enumToStringRef(dxil::ResourceClass(llvm::to_underlying(Type)),
+ ArrayRef(ResourceClassNames));
return OS;
}
diff --git a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
index 6d89fa7b1222c..4786efb9dea69 100644
--- a/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
+++ b/llvm/lib/Frontend/HLSL/RootSignatureMetadata.cpp
@@ -58,13 +58,6 @@ static const EnumEntry<dxil::ResourceClass> ResourceClassNames[] = {
{"Sampler", dxil::ResourceClass::Sampler},
};
-static std::optional<StringRef> getResourceName(dxil::ResourceClass Class) {
- for (const auto &ClassEnum : ResourceClassNames)
- if (ClassEnum.Value == Class)
- return ClassEnum.Name;
- return std::nullopt;
-}
-
namespace {
// We use the OverloadVisit with std::visit to ensure the compiler catches if a
@@ -133,10 +126,11 @@ MDNode *MetadataBuilder::BuildRootConstants(const RootConstants &Constants) {
MDNode *MetadataBuilder::BuildRootDescriptor(const RootDescriptor &Descriptor) {
IRBuilder<> Builder(Ctx);
- std::optional<StringRef> ResName =
- getResourceName(dxil::ResourceClass(to_underlying(Descriptor.Type)));
- assert(ResName && "Provided an invalid Resource Class");
- SmallString<7> Name({"Root", *ResName});
+ StringRef ResName =
+ enumToStringRef(dxil::ResourceClass(to_underlying(Descriptor.Type)),
+ ArrayRef(ResourceClassNames));
+ assert(0 < ResName.size() && "Provided an invalid Resource Class");
+ SmallString<7> Name({"Root", ResName});
Metadata *Operands[] = {
MDString::get(Ctx, Name),
ConstantAsMetadata::get(
@@ -174,11 +168,12 @@ MDNode *MetadataBuilder::BuildDescriptorTable(const DescriptorTable &Table) {
MDNode *MetadataBuilder::BuildDescriptorTableClause(
const DescriptorTableClause &Clause) {
IRBuilder<> Builder(Ctx);
- std::optional<StringRef> ResName =
- getResourceName(dxil::ResourceClass(to_underlying(Clause.Type)));
- assert(ResName && "Provided an invalid Resource Class");
+ StringRef ResName =
+ enumToStringRef(dxil::ResourceClass(to_underlying(Clause.Type)),
+ ArrayRef(ResourceClassNames));
+ assert(0 < ResName.size() && "Provided an invalid Resource Class");
Metadata *Operands[] = {
- MDString::get(Ctx, *ResName),
+ MDString::get(Ctx, ResName),
ConstantAsMetadata::get(Builder.getInt32(Clause.NumDescriptors)),
ConstantAsMetadata::get(Builder.getInt32(Clause.Reg.Number)),
ConstantAsMetadata::get(Builder.getInt32(Clause.Space)),
|
V-FEXrt
requested changes
Aug 5, 2025
| if (MaybeFlag) | ||
| OS << *MaybeFlag; | ||
| StringRef MaybeFlag = enumToStringRef(T(Bit), Flags); | ||
| if (0 < MaybeFlag.size()) |
Contributor
There was a problem hiding this comment.
this is a very strange way to write this check. Using an std::optional lets you keep the more typical if (MaybeFlag)
Contributor
There was a problem hiding this comment.
(this applies anywhere you have if (0 < MaybeFlag.size()))
Collaborator
There was a problem hiding this comment.
Alternatively the current code could be written as if( !MaybeFlag.empty())
I don't have strong feelings one way or another but empty strings for optional strings is pretty common.
Contributor
There was a problem hiding this comment.
Yeah fair enough. Checking for empty() is also fine
llvm-beanz
reviewed
Aug 6, 2025
| return utohexstr(Value, true); | ||
| } | ||
|
|
||
| template <typename T, typename TEnum> |
Collaborator
There was a problem hiding this comment.
It is probably useful to add a doxygen comment block above this API explaining that it returns an empty string if the enum doesn't have a direct mapping.
V-FEXrt
approved these changes
Aug 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce the
enumToStringRefenum intoScopedPrinter.hthat replicatesenumToStringbehaviour, expect that instead of returning a hex value string, it just returns an empty string. This allows us to return a StringRef and easily check if an invalid enum was provided based on the StringRef sizeThis then uses
enumToStringRefto remove the redundantgetResourceNameandprintEnumfunctions.Resolves: #151200.