https://github.com/inbelic updated https://github.com/llvm/llvm-project/pull/140181
>From 1c40e031725f1bd21c5c0d24d333781f99bd2a0a Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Thu, 15 May 2025 23:14:10 +0000 Subject: [PATCH 01/18] pre-req: add missing token to Lexer --- clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def | 3 +++ clang/unittests/Lex/LexHLSLRootSignatureTest.cpp | 2 ++ 2 files changed, 5 insertions(+) diff --git a/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def b/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def index ddebe82987197..5d16eaa5b72f6 100644 --- a/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def +++ b/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def @@ -100,6 +100,9 @@ KEYWORD(flags) KEYWORD(numDescriptors) KEYWORD(offset) +// StaticSampler Keywords: +KEYWORD(mipLODBias) + // Unbounded Enum: UNBOUNDED_ENUM(unbounded, "unbounded") diff --git a/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp b/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp index 3e38c281f4fb1..b610b8f10f8da 100644 --- a/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp +++ b/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp @@ -136,6 +136,8 @@ TEST_F(LexHLSLRootSignatureTest, ValidLexAllTokensTest) { space visibility flags numDescriptors offset + mipLODBias + unbounded DESCRIPTOR_RANGE_OFFSET_APPEND >From f3ace332f9984dfefea388b9bc26da0e7643c6c5 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Thu, 15 May 2025 23:15:04 +0000 Subject: [PATCH 02/18] pre-req: add parsing of MipLODBias as an uint - defines a float data member of StaticSampler that will be used to test functionality of parsing a float --- .../clang/Parse/ParseHLSLRootSignature.h | 1 + clang/lib/Parse/ParseHLSLRootSignature.cpp | 21 +++++++++++++++++++ .../Parse/ParseHLSLRootSignatureTest.cpp | 3 ++- .../llvm/Frontend/HLSL/HLSLRootSignature.h | 1 + 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index da17388a2aea5..2ec806c380b37 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -111,6 +111,7 @@ class RootSignatureParser { struct ParsedStaticSamplerParams { std::optional<llvm::hlsl::rootsig::Register> Reg; + std::optional<float> MipLODBias; }; std::optional<ParsedStaticSamplerParams> parseStaticSamplerParams(); diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index a37505e360fc0..d16644f990847 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -376,6 +376,10 @@ std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() { Sampler.Reg = Params->Reg.value(); + // Fill in optional values + if (Params->MipLODBias.has_value()) + Sampler.MipLODBias = Params->MipLODBias.value(); + if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_hlsl_unexpected_end_of_params, /*param of=*/TokenKind::kw_StaticSampler)) @@ -661,6 +665,23 @@ RootSignatureParser::parseStaticSamplerParams() { return std::nullopt; Params.Reg = Reg; } + + // `mipLODBias` `=` NUMBER + if (tryConsumeExpectedToken(TokenKind::kw_mipLODBias)) { + if (Params.MipLODBias.has_value()) { + getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param) + << CurToken.TokKind; + return std::nullopt; + } + + if (consumeExpectedToken(TokenKind::pu_equal)) + return std::nullopt; + + auto MipLODBias = parseUIntParam(); + if (!MipLODBias.has_value()) + return std::nullopt; + Params.MipLODBias = (float)*MipLODBias; + } } while (tryConsumeExpectedToken(TokenKind::pu_comma)); return Params; diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index cd3b12dbe4d24..c7dae1c27ceeb 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -225,7 +225,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) { const llvm::StringLiteral Source = R"cc( - StaticSampler(s0) + StaticSampler(s0, mipLODBias = 0) )cc"; TrivialModuleLoader ModLoader; @@ -247,6 +247,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) { ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.ViewType, RegisterType::SReg); ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.Number, 0u); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0u); ASSERT_TRUE(Consumer->isSatisfied()); } diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h index f9de86b567fea..fea9a9962991c 100644 --- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h +++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h @@ -157,6 +157,7 @@ raw_ostream &operator<<(raw_ostream &OS, const DescriptorTableClause &Clause); struct StaticSampler { Register Reg; + float MipLODBias = 0.f; }; /// Models RootElement : RootFlags | RootConstants | RootParam >From 9be00bde6ea222beff5b4f401d8fae87fd597697 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Thu, 15 May 2025 23:42:27 +0000 Subject: [PATCH 03/18] add support for parsing signed integer for float param --- .../clang/Parse/ParseHLSLRootSignature.h | 4 ++ clang/lib/Parse/ParseHLSLRootSignature.cpp | 51 ++++++++++++++++++- .../Parse/ParseHLSLRootSignatureTest.cpp | 37 +++++++++++++- 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index 2ec806c380b37..9d6c9e2225bcf 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -118,6 +118,7 @@ class RootSignatureParser { // Common parsing methods std::optional<uint32_t> parseUIntParam(); std::optional<llvm::hlsl::rootsig::Register> parseRegister(); + std::optional<float> parseFloatParam(); /// Parsing methods of various enums std::optional<llvm::hlsl::rootsig::ShaderVisibility> parseShaderVisibility(); @@ -129,6 +130,9 @@ class RootSignatureParser { /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned /// 32-bit integer std::optional<uint32_t> handleUIntLiteral(); + /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned + /// 32-bit integer + std::optional<int32_t> handleIntLiteral(bool Negated); /// Flags may specify the value of '0' to denote that there should be no /// flags set. diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index d16644f990847..c4b77f41b2942 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -677,7 +677,7 @@ RootSignatureParser::parseStaticSamplerParams() { if (consumeExpectedToken(TokenKind::pu_equal)) return std::nullopt; - auto MipLODBias = parseUIntParam(); + auto MipLODBias = parseFloatParam(); if (!MipLODBias.has_value()) return std::nullopt; Params.MipLODBias = (float)*MipLODBias; @@ -730,6 +730,30 @@ std::optional<Register> RootSignatureParser::parseRegister() { return Reg; } +std::optional<float> RootSignatureParser::parseFloatParam() { + assert(CurToken.TokKind == TokenKind::pu_equal && + "Expects to only be invoked starting at given keyword"); + // Consume sign modifier + bool Signed = tryConsumeExpectedToken({TokenKind::pu_plus, TokenKind::pu_minus}); + bool Negated = Signed && CurToken.TokKind == TokenKind::pu_minus; + + // Handle an uint and interpret it as a float + if (!Signed && tryConsumeExpectedToken(TokenKind::int_literal)) { + auto UInt = handleUIntLiteral(); + if (!UInt.has_value()) + return std::nullopt; + return (float)UInt.value(); + } else if (tryConsumeExpectedToken(TokenKind::int_literal)) { + auto Int = handleIntLiteral(Negated); + if (!Int.has_value()) + return std::nullopt; + + return (float)Int.value(); + } + + return std::nullopt; +} + std::optional<llvm::hlsl::rootsig::ShaderVisibility> RootSignatureParser::parseShaderVisibility() { assert(CurToken.TokKind == TokenKind::pu_equal && @@ -856,6 +880,31 @@ std::optional<uint32_t> RootSignatureParser::handleUIntLiteral() { return Val.getExtValue(); } +std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { + // Parse the numeric value and do semantic checks on its specification + clang::NumericLiteralParser Literal(CurToken.NumSpelling, CurToken.TokLoc, + PP.getSourceManager(), PP.getLangOpts(), + PP.getTargetInfo(), PP.getDiagnostics()); + if (Literal.hadError) + return true; // Error has already been reported so just return + + assert(Literal.isIntegerLiteral() && "IsNumberChar will only support digits"); + + llvm::APSInt Val = llvm::APSInt(32, true); + if (Literal.GetIntegerValue(Val) || INT32_MAX < Val.getExtValue()) { + // Report that the value has overflowed + PP.getDiagnostics().Report(CurToken.TokLoc, + diag::err_hlsl_number_literal_overflow) + << 0 << CurToken.NumSpelling; + return std::nullopt; + } + + if (Negated) + return static_cast<int32_t>((-Val).getExtValue()); + + return static_cast<int32_t>(Val.getExtValue()); +} + bool RootSignatureParser::verifyZeroFlag() { assert(CurToken.TokKind == TokenKind::int_literal); auto X = handleUIntLiteral(); diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index c7dae1c27ceeb..ecb7d0715eaad 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -247,7 +247,42 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) { ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.ViewType, RegisterType::SReg); ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.Number, 0u); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0u); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.f); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + +TEST_F(ParseHLSLRootSignatureTest, ValidParseFloatsTest) { + const llvm::StringLiteral Source = R"cc( + StaticSampler(s0, mipLODBias = 0), + StaticSampler(s0, mipLODBias = +1), + StaticSampler(s0, mipLODBias = -1) + )cc"; + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + auto TokLoc = SourceLocation(); + + hlsl::RootSignatureLexer Lexer(Source, TokLoc); + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(Elements, Lexer, *PP); + + // Test no diagnostics produced + Consumer->setNoDiag(); + + ASSERT_FALSE(Parser.parse()); + + RootElement Elem = Elements[0]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.f); + + Elem = Elements[1]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 1.f); + + Elem = Elements[2]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -1.f); ASSERT_TRUE(Consumer->isSatisfied()); } >From d4608c342d2872fb48195231a29507f5efe7a162 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Fri, 16 May 2025 00:20:46 +0000 Subject: [PATCH 04/18] add support for parsing a float for float param --- .../clang/Parse/ParseHLSLRootSignature.h | 4 +- clang/lib/Parse/ParseHLSLRootSignature.cpp | 60 ++++++++++++++++--- .../Parse/ParseHLSLRootSignatureTest.cpp | 42 ++++++++++++- 3 files changed, 97 insertions(+), 9 deletions(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index 9d6c9e2225bcf..dcdc7deab68c9 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -130,9 +130,11 @@ class RootSignatureParser { /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned /// 32-bit integer std::optional<uint32_t> handleUIntLiteral(); - /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned + /// Use NumericLiteralParser to convert CurToken.NumSpelling into a signed /// 32-bit integer std::optional<int32_t> handleIntLiteral(bool Negated); + /// Use NumericLiteralParser to convert CurToken.NumSpelling into a float + std::optional<float> handleFloatLiteral(bool Negated); /// Flags may specify the value of '0' to denote that there should be no /// flags set. diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index c4b77f41b2942..05636a48af534 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -6,6 +6,8 @@ // //===----------------------------------------------------------------------===// +#include <float.h> + #include "clang/Parse/ParseHLSLRootSignature.h" #include "clang/Lex/LiteralSupport.h" @@ -734,7 +736,8 @@ std::optional<float> RootSignatureParser::parseFloatParam() { assert(CurToken.TokKind == TokenKind::pu_equal && "Expects to only be invoked starting at given keyword"); // Consume sign modifier - bool Signed = tryConsumeExpectedToken({TokenKind::pu_plus, TokenKind::pu_minus}); + bool Signed = + tryConsumeExpectedToken({TokenKind::pu_plus, TokenKind::pu_minus}); bool Negated = Signed && CurToken.TokKind == TokenKind::pu_minus; // Handle an uint and interpret it as a float @@ -747,8 +750,12 @@ std::optional<float> RootSignatureParser::parseFloatParam() { auto Int = handleIntLiteral(Negated); if (!Int.has_value()) return std::nullopt; - return (float)Int.value(); + } else if (tryConsumeExpectedToken(TokenKind::float_literal)) { + auto Float = handleFloatLiteral(Negated); + if (!Float.has_value()) + return std::nullopt; + return Float.value(); } return std::nullopt; @@ -864,9 +871,10 @@ std::optional<uint32_t> RootSignatureParser::handleUIntLiteral() { PP.getSourceManager(), PP.getLangOpts(), PP.getTargetInfo(), PP.getDiagnostics()); if (Literal.hadError) - return true; // Error has already been reported so just return + return std::nullopt; // Error has already been reported so just return - assert(Literal.isIntegerLiteral() && "IsNumberChar will only support digits"); + assert(Literal.isIntegerLiteral() && + "NumSpelling can only consist of digits"); llvm::APSInt Val = llvm::APSInt(32, false); if (Literal.GetIntegerValue(Val)) { @@ -886,9 +894,10 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { PP.getSourceManager(), PP.getLangOpts(), PP.getTargetInfo(), PP.getDiagnostics()); if (Literal.hadError) - return true; // Error has already been reported so just return + return std::nullopt; // Error has already been reported so just return - assert(Literal.isIntegerLiteral() && "IsNumberChar will only support digits"); + assert(Literal.isIntegerLiteral() && + "NumSpelling can only consist of digits"); llvm::APSInt Val = llvm::APSInt(32, true); if (Literal.GetIntegerValue(Val) || INT32_MAX < Val.getExtValue()) { @@ -900,11 +909,48 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { } if (Negated) - return static_cast<int32_t>((-Val).getExtValue()); + Val = -Val; return static_cast<int32_t>(Val.getExtValue()); } +std::optional<float> RootSignatureParser::handleFloatLiteral(bool Negated) { + // Parse the numeric value and do semantic checks on its specification + clang::NumericLiteralParser Literal(CurToken.NumSpelling, CurToken.TokLoc, + PP.getSourceManager(), PP.getLangOpts(), + PP.getTargetInfo(), PP.getDiagnostics()); + if (Literal.hadError) + return std::nullopt; // Error has already been reported so just return + + assert(Literal.isFloatingLiteral() && + "NumSpelling is consistent with isNumberChar in " + "LexHLSLRootSignature.cpp"); + + // DXC used `strtod` to convert the token string to a float which corresponds + // to: + auto DXCSemantics = llvm::APFloat::Semantics::S_IEEEdouble; + auto DXCRoundingMode = llvm::RoundingMode::NearestTiesToEven; + + llvm::APFloat Val = + llvm::APFloat(llvm::APFloat::EnumToSemantics(DXCSemantics)); + llvm::APFloat::opStatus Status = Literal.GetFloatValue(Val, DXCRoundingMode); + + // The float is valid with opInexect as this just denotes if rounding occured + if (Status != llvm::APFloat::opStatus::opOK && + Status != llvm::APFloat::opStatus::opInexact) + return std::nullopt; + + if (Negated) + Val = -Val; + + double DoubleVal = Val.convertToDouble(); + if (FLT_MAX < DoubleVal || DoubleVal < -FLT_MAX) { + return std::nullopt; + } + + return static_cast<float>(DoubleVal); +} + bool RootSignatureParser::verifyZeroFlag() { assert(CurToken.TokKind == TokenKind::int_literal); auto X = handleUIntLiteral(); diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index ecb7d0715eaad..130548d8e74ae 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -256,7 +256,15 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseFloatsTest) { const llvm::StringLiteral Source = R"cc( StaticSampler(s0, mipLODBias = 0), StaticSampler(s0, mipLODBias = +1), - StaticSampler(s0, mipLODBias = -1) + StaticSampler(s0, mipLODBias = -1), + StaticSampler(s0, mipLODBias = 42.), + StaticSampler(s0, mipLODBias = +4.2), + StaticSampler(s0, mipLODBias = -.42), + StaticSampler(s0, mipLODBias = .42e+3), + StaticSampler(s0, mipLODBias = 42E-12), + StaticSampler(s0, mipLODBias = 42.f), + StaticSampler(s0, mipLODBias = 4.2F), + StaticSampler(s0, mipLODBias = 42.e+10f), )cc"; TrivialModuleLoader ModLoader; @@ -284,6 +292,38 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseFloatsTest) { ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -1.f); + Elem = Elements[3]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 42.f); + + Elem = Elements[4]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 4.2f); + + Elem = Elements[5]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -.42f); + + Elem = Elements[6]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420.f); + + Elem = Elements[7]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.000000000042f); + + Elem = Elements[8]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 42.f); + + Elem = Elements[9]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 4.2f); + + Elem = Elements[10]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420000000000.f); + ASSERT_TRUE(Consumer->isSatisfied()); } >From 5d2a90f886ecc846939fa2643ac006450068fcd0 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Fri, 16 May 2025 01:52:44 +0000 Subject: [PATCH 05/18] add testing for over/underflow and addresses other opStatus - updates the error message to account for floats and fixes the previous use cases --- .../clang/Basic/DiagnosticParseKinds.td | 6 +- clang/lib/Parse/ParseHLSLRootSignature.cpp | 38 +++++++-- .../Parse/ParseHLSLRootSignatureTest.cpp | 84 +++++++++++++++++++ 3 files changed, 120 insertions(+), 8 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td index fd525140d0482..554d70de86902 100644 --- a/clang/include/clang/Basic/DiagnosticParseKinds.td +++ b/clang/include/clang/Basic/DiagnosticParseKinds.td @@ -1856,7 +1856,11 @@ def err_hlsl_unexpected_end_of_params : Error<"expected %0 to denote end of parameters, or, another valid parameter of %1">; def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' multiple times">; def err_hlsl_rootsig_missing_param : Error<"did not specify mandatory parameter '%0'">; -def err_hlsl_number_literal_overflow : Error<"integer literal is too large to be represented as a 32-bit %select{signed |}0 integer type">; +def err_hlsl_number_literal_overflow : Error< + "%select{integer|float}0 literal is too large to be represented as a " + "%select{32-bit %select{signed|}1 integer|float}0 type">; +def err_hlsl_number_literal_underflow : Error< + "float literal has a magnitude that is too small to be represented as a float type">; def err_hlsl_rootsig_non_zero_flag : Error<"flag value is neither a literal 0 nor a named value">; } // end of Parser diagnostics diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 05636a48af534..0593a5d73cde7 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -881,7 +881,7 @@ std::optional<uint32_t> RootSignatureParser::handleUIntLiteral() { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, diag::err_hlsl_number_literal_overflow) - << 0 << CurToken.NumSpelling; + << /*integer type*/ 0 << /*is signed*/ 0; return std::nullopt; } @@ -904,7 +904,7 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, diag::err_hlsl_number_literal_overflow) - << 0 << CurToken.NumSpelling; + << /*integer type*/ 0 << /*is signed*/ 1; return std::nullopt; } @@ -923,8 +923,8 @@ std::optional<float> RootSignatureParser::handleFloatLiteral(bool Negated) { return std::nullopt; // Error has already been reported so just return assert(Literal.isFloatingLiteral() && - "NumSpelling is consistent with isNumberChar in " - "LexHLSLRootSignature.cpp"); + "NumSpelling consists only of [0-9.ef+-]. Any malformed NumSpelling " + "will be caught and reported by NumericLiteralParser."); // DXC used `strtod` to convert the token string to a float which corresponds // to: @@ -935,16 +935,40 @@ std::optional<float> RootSignatureParser::handleFloatLiteral(bool Negated) { llvm::APFloat(llvm::APFloat::EnumToSemantics(DXCSemantics)); llvm::APFloat::opStatus Status = Literal.GetFloatValue(Val, DXCRoundingMode); - // The float is valid with opInexect as this just denotes if rounding occured - if (Status != llvm::APFloat::opStatus::opOK && - Status != llvm::APFloat::opStatus::opInexact) + // Note: we do not error when opStatus::opInexact by itself as this just + // denotes that rounding occured but not that it is invalid + assert(!(Status & llvm::APFloat::opStatus::opInvalidOp) && + "NumSpelling consists only of [0-9.ef+-]. Any malformed NumSpelling " + "will be caught and reported by NumericLiteralParser."); + + assert(!(Status & llvm::APFloat::opStatus::opDivByZero) && + "It is not possible for a division to be performed when " + "constructing an APFloat from a string"); + + if (Status & llvm::APFloat::opStatus::opUnderflow) { + // Report that the value has underflowed + PP.getDiagnostics().Report(CurToken.TokLoc, + diag::err_hlsl_number_literal_underflow); return std::nullopt; + } + + if (Status & llvm::APFloat::opStatus::opOverflow) { + // Report that the value has overflowed + PP.getDiagnostics().Report(CurToken.TokLoc, + diag::err_hlsl_number_literal_overflow) + << /*float type*/ 1; + return std::nullopt; + } if (Negated) Val = -Val; double DoubleVal = Val.convertToDouble(); if (FLT_MAX < DoubleVal || DoubleVal < -FLT_MAX) { + // Report that the value has overflowed + PP.getDiagnostics().Report(CurToken.TokLoc, + diag::err_hlsl_number_literal_overflow) + << /*float type*/ 1; return std::nullopt; } diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 130548d8e74ae..894c707313eac 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -793,6 +793,90 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedNumberTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedFloatTest) { + // This test will check that the lexing fails due to a float overflow + const llvm::StringLiteral Source = R"cc( + StaticSampler(s0, mipLODBias = 3.402823467e+38F) + )cc"; + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + auto TokLoc = SourceLocation(); + + hlsl::RootSignatureLexer Lexer(Source, TokLoc); + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(Elements, Lexer, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_number_literal_overflow); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + +TEST_F(ParseHLSLRootSignatureTest, InvalidLexNegOverflowedFloatTest) { + // This test will check that the lexing fails due to negative float overflow + const llvm::StringLiteral Source = R"cc( + StaticSampler(s0, mipLODBias = -3.402823467e+38F) + )cc"; + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + auto TokLoc = SourceLocation(); + + hlsl::RootSignatureLexer Lexer(Source, TokLoc); + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(Elements, Lexer, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_number_literal_overflow); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + +TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedDoubleTest) { + // This test will check that the lexing fails due to an overflow of double + const llvm::StringLiteral Source = R"cc( + StaticSampler(s0, mipLODBias = 1.e+500) + )cc"; + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + auto TokLoc = SourceLocation(); + + hlsl::RootSignatureLexer Lexer(Source, TokLoc); + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(Elements, Lexer, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_number_literal_overflow); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + +TEST_F(ParseHLSLRootSignatureTest, InvalidLexUnderflowFloatTest) { + // This test will check that the lexing fails due to double underflow + const llvm::StringLiteral Source = R"cc( + StaticSampler(s0, mipLODBias = 10e-309) + )cc"; + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + auto TokLoc = SourceLocation(); + + hlsl::RootSignatureLexer Lexer(Source, TokLoc); + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(Elements, Lexer, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_number_literal_underflow); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + TEST_F(ParseHLSLRootSignatureTest, InvalidNonZeroFlagsTest) { // This test will check that parsing fails when a non-zero integer literal // is given to flags >From 80628f5b337725aaf95020e85a811f68beee043a Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Fri, 16 May 2025 03:32:07 +0000 Subject: [PATCH 06/18] self-review: treat postive integer as unsigned --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 0593a5d73cde7..bebb86f001794 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -740,8 +740,8 @@ std::optional<float> RootSignatureParser::parseFloatParam() { tryConsumeExpectedToken({TokenKind::pu_plus, TokenKind::pu_minus}); bool Negated = Signed && CurToken.TokKind == TokenKind::pu_minus; - // Handle an uint and interpret it as a float - if (!Signed && tryConsumeExpectedToken(TokenKind::int_literal)) { + // DXC will treat a postive signed integer as unsigned + if (!Negated && tryConsumeExpectedToken(TokenKind::int_literal)) { auto UInt = handleUIntLiteral(); if (!UInt.has_value()) return std::nullopt; >From 001ea4ddda36a8f84724fadab521cc9b4b18432a Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Mon, 26 May 2025 21:22:41 +0000 Subject: [PATCH 07/18] review: remove INT32_MAX constraint --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 2 +- clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index bebb86f001794..1dac305d75379 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -900,7 +900,7 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { "NumSpelling can only consist of digits"); llvm::APSInt Val = llvm::APSInt(32, true); - if (Literal.GetIntegerValue(Val) || INT32_MAX < Val.getExtValue()) { + if (Literal.GetIntegerValue(Val)) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, diag::err_hlsl_number_literal_overflow) diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 894c707313eac..7cb0eb9e1e473 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -265,6 +265,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseFloatsTest) { StaticSampler(s0, mipLODBias = 42.f), StaticSampler(s0, mipLODBias = 4.2F), StaticSampler(s0, mipLODBias = 42.e+10f), + StaticSampler(s0, mipLODBias = -2147483648), + StaticSampler(s0, mipLODBias = 2147483648), )cc"; TrivialModuleLoader ModLoader; @@ -324,6 +326,14 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseFloatsTest) { ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420000000000.f); + Elem = Elements[11]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -2147483648.f); + + Elem = Elements[12]; + ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 2147483648.f); + ASSERT_TRUE(Consumer->isSatisfied()); } >From 64e13dfb996e9d1acd6821bcd206d83db759b89e Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Mon, 26 May 2025 21:23:17 +0000 Subject: [PATCH 08/18] self-review: remove lingering cast --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 1dac305d75379..d5d2fab65d018 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -682,7 +682,7 @@ RootSignatureParser::parseStaticSamplerParams() { auto MipLODBias = parseFloatParam(); if (!MipLODBias.has_value()) return std::nullopt; - Params.MipLODBias = (float)*MipLODBias; + Params.MipLODBias = MipLODBias; } } while (tryConsumeExpectedToken(TokenKind::pu_comma)); >From c3f460485f5f18766805de601c13dbc27a4d79af Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Mon, 26 May 2025 21:25:26 +0000 Subject: [PATCH 09/18] self-review: use proper float comparison --- .../Parse/ParseHLSLRootSignatureTest.cpp | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 7cb0eb9e1e473..642f4283f5acb 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -247,7 +247,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) { ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.ViewType, RegisterType::SReg); ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.Number, 0u); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.f); ASSERT_TRUE(Consumer->isSatisfied()); } @@ -284,47 +284,47 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseFloatsTest) { RootElement Elem = Elements[0]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.f); Elem = Elements[1]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 1.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 1.f); Elem = Elements[2]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -1.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -1.f); Elem = Elements[3]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 42.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 42.f); Elem = Elements[4]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 4.2f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 4.2f); Elem = Elements[5]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -.42f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, -.42f); Elem = Elements[6]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420.f); Elem = Elements[7]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.000000000042f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 0.000000000042f); Elem = Elements[8]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 42.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 42.f); Elem = Elements[9]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 4.2f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 4.2f); Elem = Elements[10]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); - ASSERT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420000000000.f); + ASSERT_FLOAT_EQ(std::get<StaticSampler>(Elem).MipLODBias, 420000000000.f); Elem = Elements[11]; ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem)); >From 211850abed102f41aa9ad994b176499882f30208 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Mon, 26 May 2025 22:31:08 +0000 Subject: [PATCH 10/18] review: add constraint on magnitude of signed integer --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 11 +++++++--- .../Parse/ParseHLSLRootSignatureTest.cpp | 22 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index d5d2fab65d018..8341e6a62f098 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -876,7 +876,7 @@ std::optional<uint32_t> RootSignatureParser::handleUIntLiteral() { assert(Literal.isIntegerLiteral() && "NumSpelling can only consist of digits"); - llvm::APSInt Val = llvm::APSInt(32, false); + llvm::APSInt Val = llvm::APSInt(32, /*IsUnsigned=*/true); if (Literal.GetIntegerValue(Val)) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, @@ -899,8 +899,13 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { assert(Literal.isIntegerLiteral() && "NumSpelling can only consist of digits"); - llvm::APSInt Val = llvm::APSInt(32, true); - if (Literal.GetIntegerValue(Val)) { + llvm::APSInt Val = llvm::APSInt(32, /*IsUnsigned=*/true); + // GetIntegerValue will overwrite Val from the parsed Literal and return + // true if it overflows as a 32-bit unsigned int. Then check that it also + // doesn't overflow as a signed 32-bit int. + int64_t MaxMagnitude = + -static_cast<int64_t>(std::numeric_limits<int32_t>::min()); + if (Literal.GetIntegerValue(Val) || MaxMagnitude < Val.getExtValue()) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, diag::err_hlsl_number_literal_overflow) diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 642f4283f5acb..b08c07f41141e 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -803,6 +803,28 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedNumberTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidParseOverflowedNegativeNumberTest) { + // This test will check that parsing fails due to a unsigned integer having + // too large of a magnitude to be interpreted as its negative + const llvm::StringLiteral Source = R"cc( + StaticSampler(s0, mipLODBias = -4294967295) + )cc"; + + TrivialModuleLoader ModLoader; + auto PP = createPP(Source, ModLoader); + auto TokLoc = SourceLocation(); + + hlsl::RootSignatureLexer Lexer(Source, TokLoc); + SmallVector<RootElement> Elements; + hlsl::RootSignatureParser Parser(Elements, Lexer, *PP); + + // Test correct diagnostic produced + Consumer->setExpected(diag::err_hlsl_number_literal_overflow); + ASSERT_TRUE(Parser.parse()); + + ASSERT_TRUE(Consumer->isSatisfied()); +} + TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedFloatTest) { // This test will check that the lexing fails due to a float overflow const llvm::StringLiteral Source = R"cc( >From 265c72961bacdb5798eadc5ac927aeee14569bab Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:42:25 +0000 Subject: [PATCH 11/18] review: remove use of float.h for numeric_limits --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 8341e6a62f098..0ae96a8db89e4 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -6,8 +6,6 @@ // //===----------------------------------------------------------------------===// -#include <float.h> - #include "clang/Parse/ParseHLSLRootSignature.h" #include "clang/Lex/LiteralSupport.h" @@ -969,7 +967,8 @@ std::optional<float> RootSignatureParser::handleFloatLiteral(bool Negated) { Val = -Val; double DoubleVal = Val.convertToDouble(); - if (FLT_MAX < DoubleVal || DoubleVal < -FLT_MAX) { + double FloatMax = double(std::numeric_limits<float>::max()); + if (FloatMax < DoubleVal || DoubleVal < -FloatMax) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, diag::err_hlsl_number_literal_overflow) >From ba4e2ae7b7badfc1ec0382e0a3cd46f6013affd5 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:43:45 +0000 Subject: [PATCH 12/18] review: spell out types --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 0ae96a8db89e4..771cb38b0996e 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -740,17 +740,17 @@ std::optional<float> RootSignatureParser::parseFloatParam() { // DXC will treat a postive signed integer as unsigned if (!Negated && tryConsumeExpectedToken(TokenKind::int_literal)) { - auto UInt = handleUIntLiteral(); + std::optional<uint32_t> UInt = handleUIntLiteral(); if (!UInt.has_value()) return std::nullopt; return (float)UInt.value(); } else if (tryConsumeExpectedToken(TokenKind::int_literal)) { - auto Int = handleIntLiteral(Negated); + std::optional<int32_t> = handleIntLiteral(Negated); if (!Int.has_value()) return std::nullopt; return (float)Int.value(); } else if (tryConsumeExpectedToken(TokenKind::float_literal)) { - auto Float = handleFloatLiteral(Negated); + std::optional<float> Float = handleFloatLiteral(Negated); if (!Float.has_value()) return std::nullopt; return Float.value(); >From d5fbc7ca3771d5149d35fe6b53bde7cafddd8394 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:46:41 +0000 Subject: [PATCH 13/18] review: don't use else after return --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 771cb38b0996e..ad26d18736e6b 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -744,12 +744,16 @@ std::optional<float> RootSignatureParser::parseFloatParam() { if (!UInt.has_value()) return std::nullopt; return (float)UInt.value(); - } else if (tryConsumeExpectedToken(TokenKind::int_literal)) { + } + + if (Negated && tryConsumeExpectedToken(TokenKind::int_literal)) { std::optional<int32_t> = handleIntLiteral(Negated); if (!Int.has_value()) return std::nullopt; return (float)Int.value(); - } else if (tryConsumeExpectedToken(TokenKind::float_literal)) { + } + + if (tryConsumeExpectedToken(TokenKind::float_literal)) { std::optional<float> Float = handleFloatLiteral(Negated); if (!Float.has_value()) return std::nullopt; >From ff0791f29cbf0ad310d8e4db91b7c576e9a264dd Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:47:21 +0000 Subject: [PATCH 14/18] self-review: fix typo --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index ad26d18736e6b..a50ff0156086a 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -747,7 +747,7 @@ std::optional<float> RootSignatureParser::parseFloatParam() { } if (Negated && tryConsumeExpectedToken(TokenKind::int_literal)) { - std::optional<int32_t> = handleIntLiteral(Negated); + std::optional<int32_t> Int = handleIntLiteral(Negated); if (!Int.has_value()) return std::nullopt; return (float)Int.value(); >From a2174da0f65794fddfbd1d29389de8b889031dff Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:49:12 +0000 Subject: [PATCH 15/18] review: use c++ style casting --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index a50ff0156086a..244a390a8f04c 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -743,14 +743,14 @@ std::optional<float> RootSignatureParser::parseFloatParam() { std::optional<uint32_t> UInt = handleUIntLiteral(); if (!UInt.has_value()) return std::nullopt; - return (float)UInt.value(); + return float(UInt.value()); } if (Negated && tryConsumeExpectedToken(TokenKind::int_literal)) { std::optional<int32_t> Int = handleIntLiteral(Negated); if (!Int.has_value()) return std::nullopt; - return (float)Int.value(); + return float(Int.value()); } if (tryConsumeExpectedToken(TokenKind::float_literal)) { @@ -905,8 +905,7 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { // GetIntegerValue will overwrite Val from the parsed Literal and return // true if it overflows as a 32-bit unsigned int. Then check that it also // doesn't overflow as a signed 32-bit int. - int64_t MaxMagnitude = - -static_cast<int64_t>(std::numeric_limits<int32_t>::min()); + int64_t MaxMagnitude = -int64_t(std::numeric_limits<int32_t>::min()); if (Literal.GetIntegerValue(Val) || MaxMagnitude < Val.getExtValue()) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, @@ -918,7 +917,7 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { if (Negated) Val = -Val; - return static_cast<int32_t>(Val.getExtValue()); + return int32_t(Val.getExtValue()); } std::optional<float> RootSignatureParser::handleFloatLiteral(bool Negated) { >From e60ec79ee635afb367a765e7816e7da97e9178fb Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:51:19 +0000 Subject: [PATCH 16/18] review: initialize directly --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 244a390a8f04c..d33a4ee58bd92 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -878,7 +878,7 @@ std::optional<uint32_t> RootSignatureParser::handleUIntLiteral() { assert(Literal.isIntegerLiteral() && "NumSpelling can only consist of digits"); - llvm::APSInt Val = llvm::APSInt(32, /*IsUnsigned=*/true); + llvm::APSInt Val(32, /*IsUnsigned=*/true); if (Literal.GetIntegerValue(Val)) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, @@ -901,7 +901,7 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { assert(Literal.isIntegerLiteral() && "NumSpelling can only consist of digits"); - llvm::APSInt Val = llvm::APSInt(32, /*IsUnsigned=*/true); + llvm::APSInt Val(32, /*IsUnsigned=*/true); // GetIntegerValue will overwrite Val from the parsed Literal and return // true if it overflows as a 32-bit unsigned int. Then check that it also // doesn't overflow as a signed 32-bit int. @@ -937,9 +937,8 @@ std::optional<float> RootSignatureParser::handleFloatLiteral(bool Negated) { auto DXCSemantics = llvm::APFloat::Semantics::S_IEEEdouble; auto DXCRoundingMode = llvm::RoundingMode::NearestTiesToEven; - llvm::APFloat Val = - llvm::APFloat(llvm::APFloat::EnumToSemantics(DXCSemantics)); - llvm::APFloat::opStatus Status = Literal.GetFloatValue(Val, DXCRoundingMode); + llvm::APFloat Val(llvm::APFloat::EnumToSemantics(DXCSemantics)); + llvm::APFloat::opStatus Status(Literal.GetFloatValue(Val, DXCRoundingMode)); // Note: we do not error when opStatus::opInexact by itself as this just // denotes that rounding occured but not that it is invalid >From b507dbaca10330b4544b931fe3cc45198f954477 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 16:56:54 +0000 Subject: [PATCH 17/18] review: add explicit comment of how the float is handled --- clang/include/clang/Parse/ParseHLSLRootSignature.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index dcdc7deab68c9..a03f33dfd3b4e 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -134,6 +134,14 @@ class RootSignatureParser { /// 32-bit integer std::optional<int32_t> handleIntLiteral(bool Negated); /// Use NumericLiteralParser to convert CurToken.NumSpelling into a float + /// + /// This matches the behaviour of DXC, which is as follows: + /// - convert the spelling with `strtod` + /// - check for a float overflow + /// - cast the double to a float + /// The behaviour of `strtod` is replicated using: + /// Semantics: llvm::APFloat::Semantics::S_IEEEdouble + /// RoundingMode: llvm::RoundingMode::NearestTiesToEven std::optional<float> handleFloatLiteral(bool Negated); /// Flags may specify the value of '0' to denote that there should be no >From 12e972c1d456624c4bdce46417312f4df0b6f77f Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 28 May 2025 17:03:31 +0000 Subject: [PATCH 18/18] review: let `handleIntLiteral` properly handle a signed positive integer - this code path will currently not ever get evaluated but for the sake of expected behaviour of a future user we will add this functionality --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index d33a4ee58bd92..8be91f5991b21 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -903,10 +903,17 @@ std::optional<int32_t> RootSignatureParser::handleIntLiteral(bool Negated) { llvm::APSInt Val(32, /*IsUnsigned=*/true); // GetIntegerValue will overwrite Val from the parsed Literal and return - // true if it overflows as a 32-bit unsigned int. Then check that it also - // doesn't overflow as a signed 32-bit int. - int64_t MaxMagnitude = -int64_t(std::numeric_limits<int32_t>::min()); - if (Literal.GetIntegerValue(Val) || MaxMagnitude < Val.getExtValue()) { + // true if it overflows as a 32-bit unsigned int + bool Overflowed = Literal.GetIntegerValue(Val); + + // So we then need to check that it doesn't overflow as a 32-bit signed int: + int64_t MaxNegativeMagnitude = -int64_t(std::numeric_limits<int32_t>::min()); + Overflowed |= (Negated && MaxNegativeMagnitude < Val.getExtValue()); + + int64_t MaxPositiveMagnitude = int64_t(std::numeric_limits<int32_t>::max()); + Overflowed |= (!Negated && MaxPositiveMagnitude < Val.getExtValue()); + + if (Overflowed) { // Report that the value has overflowed PP.getDiagnostics().Report(CurToken.TokLoc, diag::err_hlsl_number_literal_overflow) _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
