From d2555997f321206a622ae7c9373aadd7a3b0214b Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 17 Apr 2024 16:50:13 -0400 Subject: [PATCH 1/5] [DxilOp] recover old behavior of OP::GetOpFunc This change change move IsOverloadLegal check in OP::GetOpFunc back into DXASSERT. It will allow illegal dxil op generated for release build. Then validation should catch those illegal dxil ops if they're not optimized in SimplifyDxilCall. Fixes #6410 --- lib/DXIL/DxilOperations.cpp | 5 +++-- .../types/intrinsic_call_with_all_literal_args.hlsl | 12 ++++++++++++ tools/clang/test/lit.cfg | 5 +++++ 3 files changed, 20 insertions(+), 2 deletions(-) create mode 100644 tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl diff --git a/lib/DXIL/DxilOperations.cpp b/lib/DXIL/DxilOperations.cpp index b571601dc4..636260c486 100644 --- a/lib/DXIL/DxilOperations.cpp +++ b/lib/DXIL/DxilOperations.cpp @@ -3564,8 +3564,9 @@ void OP::UpdateCache(OpCodeClass opClass, Type *Ty, llvm::Function *F) { Function *OP::GetOpFunc(OpCode opCode, Type *pOverloadType) { if (opCode == OpCode::NumOpCodes) return nullptr; - if (!IsOverloadLegal(opCode, pOverloadType)) - return nullptr; + DXASSERT(IsOverloadLegal(opCode, pOverloadType), + "otherwise the caller requested illegal operation overload (eg HLSL " + "function with unsupported types for mapped intrinsic function)"); OpCodeClass opClass = m_OpCodeProps[(unsigned)opCode].opCodeClass; Function *&F = diff --git a/tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl b/tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl new file mode 100644 index 0000000000..fc2fdea459 --- /dev/null +++ b/tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl @@ -0,0 +1,12 @@ +// RUN: %dxc -Tps_6_0 %s | FileCheck %s + +// NOTE, this will generate illegal dxil operation which will be optimized later. +// Limited to release build because debug build will hit assert. +// REQUIRES: release_build + +// Make sure literal argument works for fmod. +// CHECK: call void @dx.op.storeOutput.f32(i32 5, i32 0, i32 0, i8 0, float 1.000000e+00) + +float main() : SV_Target { + return fmod(1.0, 2.0); +} diff --git a/tools/clang/test/lit.cfg b/tools/clang/test/lit.cfg index e42297d2c9..f80d39609f 100644 --- a/tools/clang/test/lit.cfg +++ b/tools/clang/test/lit.cfg @@ -503,6 +503,11 @@ if config.enable_backtrace == "1": if config.spirv: config.available_features.add("spirv") +build_mode = lit_config.params.get('build_mode', None) + +if build_mode == "Release" or build_mode == "RelWithDebInfo": + config.available_features.add("release_build") + # Check supported dxil version def get_dxil_version(): result = subprocess.run([lit.util.which('dxc', llvm_tools_dir), "--version"], stdout=subprocess.PIPE) From d863aa3b46f7b91a9cb9a02c53de1f8b8fb31222 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 17 Apr 2024 18:15:08 -0400 Subject: [PATCH 2/5] Remove the assert. --- lib/DXIL/DxilOperations.cpp | 13 ++++-- .../intrinsic_call_with_all_literal_args.hlsl | 12 ------ .../CodeGenDXIL/literal/fmod_const_eval.hlsl | 43 +++++++++++++++++++ .../literal/length_const_eval.hlsl | 41 ++++++++++++++++++ .../literal/normalize_const_eval.hlsl | 40 +++++++++++++++++ tools/clang/test/lit.cfg | 5 --- 6 files changed, 134 insertions(+), 20 deletions(-) delete mode 100644 tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl create mode 100644 tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl create mode 100644 tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl create mode 100644 tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl diff --git a/lib/DXIL/DxilOperations.cpp b/lib/DXIL/DxilOperations.cpp index 636260c486..dbc54b2028 100644 --- a/lib/DXIL/DxilOperations.cpp +++ b/lib/DXIL/DxilOperations.cpp @@ -3564,9 +3564,16 @@ void OP::UpdateCache(OpCodeClass opClass, Type *Ty, llvm::Function *F) { Function *OP::GetOpFunc(OpCode opCode, Type *pOverloadType) { if (opCode == OpCode::NumOpCodes) return nullptr; - DXASSERT(IsOverloadLegal(opCode, pOverloadType), - "otherwise the caller requested illegal operation overload (eg HLSL " - "function with unsupported types for mapped intrinsic function)"); + // Remove this assert on illegal overload for now. + // Illegal overloads are generated and eliminated by DXIL op constant + // evaluation for a number of cases where a double overload of an HL intrinsic + // that otherwise does not support double is used for literal values, when + // there is no constant evaluation for the intrinsic in CodeGen. + // Illegal overloads of DXIL intrinsics may survive through to final DXIL, + // but these will be caught by the validator, and this is not a regression. + // DXASSERT(IsOverloadLegal(opCode, pOverloadType), + // "otherwise the caller requested illegal operation overload (eg HLSL " + // "function with unsupported types for mapped intrinsic function)"); OpCodeClass opClass = m_OpCodeProps[(unsigned)opCode].opCodeClass; Function *&F = diff --git a/tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl b/tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl deleted file mode 100644 index fc2fdea459..0000000000 --- a/tools/clang/test/CodeGenDXIL/hlsl/types/intrinsic_call_with_all_literal_args.hlsl +++ /dev/null @@ -1,12 +0,0 @@ -// RUN: %dxc -Tps_6_0 %s | FileCheck %s - -// NOTE, this will generate illegal dxil operation which will be optimized later. -// Limited to release build because debug build will hit assert. -// REQUIRES: release_build - -// Make sure literal argument works for fmod. -// CHECK: call void @dx.op.storeOutput.f32(i32 5, i32 0, i32 0, i8 0, float 1.000000e+00) - -float main() : SV_Target { - return fmod(1.0, 2.0); -} diff --git a/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl new file mode 100644 index 0000000000..bcd1b794af --- /dev/null +++ b/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl @@ -0,0 +1,43 @@ +// RUN: %dxc -T vs_6_0 %s -E main | FileCheck %s +// RUN: not %dxc -T vs_6_0 %s -E main -DNO_FOLD 2>&1 | FileCheck %s --check-prefixes=NO_FOLD + +// The code path for this case asserts, but the assert does not indicate a +// serious problem with internal state. The invalid overload will either be +// constant folded away, or caught by the validator. + +// Ensure fmod is constant evaluated during codegen, or dxil const eval +// TODO: handle fp specials properly! + +RWBuffer results : register(u0); + +[shader("vertex")] +void main(bool b : B) { + uint i = 0; + + // Literal float + // 2.5, -2.5, 2.5, -2.5 + // CHECK: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 0, i32 undef, float 2.500000e+00, float -2.500000e+00, float 2.500000e+00, float -2.500000e+00, i8 15) + results[i++] = float4(fmod(5.5, 3.0), + fmod(-5.5, 3.0), + fmod(5.5, -3.0), + fmod(-5.5, -3.0)); + + // Explicit float + // 2.5, -2.5, 2.5, -2.5 + // CHECK: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 1, i32 undef, float 2.500000e+00, float -2.500000e+00, float 2.500000e+00, float -2.500000e+00, i8 15) + results[i++] = float4(fmod(5.5f, 3.0f), + fmod(-5.5f, 3.0f), + fmod(5.5f, -3.0f), + fmod(-5.5f, -3.0f)); + +#ifdef NO_FOLD + // Currently, we rely on constant folding of DXIL ops to get rid of illegal + // double overloads. If this doesn't happen, we expect a validation error. + // Ternary operator can return literal type, while not being foldable due + // non-constant condition. + // NO_FOLD: error: validation errors + // NO_FOLD: error: DXIL intrinsic overload must be valid. + float result = fmod(-5.5, b ? 1.5 : 0.5); + results[i++] = float4(result, 0, 0, 0); +#endif // NO_FOLD +} diff --git a/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl new file mode 100644 index 0000000000..dfd09aa03b --- /dev/null +++ b/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl @@ -0,0 +1,41 @@ +// RUN: %dxc -T vs_6_0 %s -E main | FileCheck %s +// RUN: not %dxc -T vs_6_0 %s -E main -DNO_FOLD 2>&1 | FileCheck %s --check-prefixes=NO_FOLD + +// The code path for this case asserts, but the assert does not indicate a +// serious problem with internal state. The invalid overload will either be +// constant folded away, or caught by the validator. + +// Ensure length is constant evaluated during codegen, or dxil const eval +// TODO: handle fp specials properly! + +RWBuffer results : register(u0); + +[shader("vertex")] +void main(bool b : B) { + uint i = 0; + + // Literal float + // CHECK: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 0, i32 undef, float 0x3FE6A09E60000000, float 0x4004C8DC20000000, float 0.000000e+00, float 0.000000e+00, i8 15) + results[i++] = float4(length(0.5.xx), + length(-1.5.xxx), + length(0.0.xxxx), + length(-0.0.xxxx)); + + // Explicit float + // CHECK: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 1, i32 undef, float 0x3FE6A09E60000000, float 0x4004C8DC20000000, float 0.000000e+00, float 0.000000e+00, i8 15) + results[i++] = float4(length(0.5F.xx), + length(-1.5F.xxx), + length(0.0F.xxxx), + length(-0.0F.xxxx)); + +#ifdef NO_FOLD + // Currently, we rely on constant folding of DXIL ops to get rid of illegal + // double overloads. If this doesn't happen, we expect a validation error. + // Ternary operator can return literal type, while not being foldable due + // non-constant condition. + // NO_FOLD: error: validation errors + // NO_FOLD: error: DXIL intrinsic overload must be valid. + float result = length((b ? 1.5 : 0.5).xxx); + results[i++] = float4(result, 0, 0, 0); +#endif // NO_FOLD +} diff --git a/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl new file mode 100644 index 0000000000..dadaf4ef30 --- /dev/null +++ b/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl @@ -0,0 +1,40 @@ +// RUN: %dxc -T vs_6_0 %s -E main | FileCheck %s +// RUN: not %dxc -T vs_6_0 %s -E main -DNO_FOLD 2>&1 | FileCheck %s --check-prefixes=NO_FOLD + +// The code path for this case asserts, but the assert does not indicate a +// serious problem with internal state. The invalid overload will either be +// constant folded away, or caught by the validator. + +// Ensure normalize is constant evaluated during codegen, or dxil const eval +// TODO: handle fp specials properly! + +RWBuffer results : register(u0); + +[shader("vertex")] +void main(bool b : B) { + uint i = 0; + + // Literal float + // CHECK: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 0, i32 undef, float 0x3FE6A09E60000000, float 0xBFE279A740000000, float 0x7FF8000000000000, float 0x7FF8000000000000, i8 15) + results[i++] = float4(normalize(0.5.xx).x, + normalize(-1.5.xxx).x, + normalize(0.0.xxxx).x, + normalize(-0.0.xxxx).x); + + // Explicit float + // CHECK: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 1, i32 undef, float 0x3FE6A09E60000000, float 0xBFE279A740000000, float 0x7FF8000000000000, float 0x7FF8000000000000, i8 15) + results[i++] = float4(normalize(0.5F.xx).x, + normalize(-1.5F.xxx).x, + normalize(0.0F.xxxx).x, + normalize(-0.0F.xxxx).x); + +#ifdef NO_FOLD + // Currently, we rely on constant folding of DXIL ops to get rid of illegal + // double overloads. If this doesn't happen, we expect a validation error. + // Ternary operator can return literal type, while not being foldable due + // non-constant condition. + // NO_FOLD: error: validation errors + // NO_FOLD: error: DXIL intrinsic overload must be valid. + results[i++] = normalize((b ? 1.25 : 2.5).xxxx); +#endif // NO_FOLD +} diff --git a/tools/clang/test/lit.cfg b/tools/clang/test/lit.cfg index f80d39609f..e42297d2c9 100644 --- a/tools/clang/test/lit.cfg +++ b/tools/clang/test/lit.cfg @@ -503,11 +503,6 @@ if config.enable_backtrace == "1": if config.spirv: config.available_features.add("spirv") -build_mode = lit_config.params.get('build_mode', None) - -if build_mode == "Release" or build_mode == "RelWithDebInfo": - config.available_features.add("release_build") - # Check supported dxil version def get_dxil_version(): result = subprocess.run([lit.util.which('dxc', llvm_tools_dir), "--version"], stdout=subprocess.PIPE) From d1bc9cfcb0b666faea95d8abd06875433ddace81 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 17 Apr 2024 19:11:13 -0400 Subject: [PATCH 3/5] Ignore null overload type. --- lib/DXIL/DxilOperations.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/DXIL/DxilOperations.cpp b/lib/DXIL/DxilOperations.cpp index dbc54b2028..dacb93ba2f 100644 --- a/lib/DXIL/DxilOperations.cpp +++ b/lib/DXIL/DxilOperations.cpp @@ -3564,6 +3564,8 @@ void OP::UpdateCache(OpCodeClass opClass, Type *Ty, llvm::Function *F) { Function *OP::GetOpFunc(OpCode opCode, Type *pOverloadType) { if (opCode == OpCode::NumOpCodes) return nullptr; + if (!pOverloadType) + return nullptr; // Remove this assert on illegal overload for now. // Illegal overloads are generated and eliminated by DXIL op constant // evaluation for a number of cases where a double overload of an HL intrinsic From 5f418c89a6854d6adaf77f879b01626b6c2d247b Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Mon, 22 Apr 2024 14:11:27 -0400 Subject: [PATCH 4/5] Update comments in tests. --- tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl | 3 +-- tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl | 3 +-- tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl index bcd1b794af..ff981ea123 100644 --- a/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl +++ b/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl @@ -1,8 +1,7 @@ // RUN: %dxc -T vs_6_0 %s -E main | FileCheck %s // RUN: not %dxc -T vs_6_0 %s -E main -DNO_FOLD 2>&1 | FileCheck %s --check-prefixes=NO_FOLD -// The code path for this case asserts, but the assert does not indicate a -// serious problem with internal state. The invalid overload will either be +// The code path generates invalid overload. The invalid overload will either be // constant folded away, or caught by the validator. // Ensure fmod is constant evaluated during codegen, or dxil const eval diff --git a/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl index dfd09aa03b..9b60ae95cb 100644 --- a/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl +++ b/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl @@ -1,8 +1,7 @@ // RUN: %dxc -T vs_6_0 %s -E main | FileCheck %s // RUN: not %dxc -T vs_6_0 %s -E main -DNO_FOLD 2>&1 | FileCheck %s --check-prefixes=NO_FOLD -// The code path for this case asserts, but the assert does not indicate a -// serious problem with internal state. The invalid overload will either be +// The code path generates invalid overload. The invalid overload will either be // constant folded away, or caught by the validator. // Ensure length is constant evaluated during codegen, or dxil const eval diff --git a/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl index dadaf4ef30..b8d14d3fcc 100644 --- a/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl +++ b/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl @@ -1,8 +1,7 @@ // RUN: %dxc -T vs_6_0 %s -E main | FileCheck %s // RUN: not %dxc -T vs_6_0 %s -E main -DNO_FOLD 2>&1 | FileCheck %s --check-prefixes=NO_FOLD -// The code path for this case asserts, but the assert does not indicate a -// serious problem with internal state. The invalid overload will either be +// The code path generates invalid overload. The invalid overload will either be // constant folded away, or caught by the validator. // Ensure normalize is constant evaluated during codegen, or dxil const eval From 490772ce4e334196b28716a04e6c24fa5a729ac0 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Fri, 26 Apr 2024 15:31:26 -0400 Subject: [PATCH 5/5] Update comments. --- lib/DXIL/DxilOperations.cpp | 4 ---- .../CodeGenDXIL/literal/fmod_const_eval.hlsl | 22 ++++++++++++++++++- .../literal/length_const_eval.hlsl | 2 +- .../literal/normalize_const_eval.hlsl | 3 ++- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/lib/DXIL/DxilOperations.cpp b/lib/DXIL/DxilOperations.cpp index dacb93ba2f..552f72e425 100644 --- a/lib/DXIL/DxilOperations.cpp +++ b/lib/DXIL/DxilOperations.cpp @@ -3566,16 +3566,12 @@ Function *OP::GetOpFunc(OpCode opCode, Type *pOverloadType) { return nullptr; if (!pOverloadType) return nullptr; - // Remove this assert on illegal overload for now. // Illegal overloads are generated and eliminated by DXIL op constant // evaluation for a number of cases where a double overload of an HL intrinsic // that otherwise does not support double is used for literal values, when // there is no constant evaluation for the intrinsic in CodeGen. // Illegal overloads of DXIL intrinsics may survive through to final DXIL, // but these will be caught by the validator, and this is not a regression. - // DXASSERT(IsOverloadLegal(opCode, pOverloadType), - // "otherwise the caller requested illegal operation overload (eg HLSL " - // "function with unsupported types for mapped intrinsic function)"); OpCodeClass opClass = m_OpCodeProps[(unsigned)opCode].opCodeClass; Function *&F = diff --git a/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl index ff981ea123..bff10c2a2e 100644 --- a/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl +++ b/tools/clang/test/CodeGenDXIL/literal/fmod_const_eval.hlsl @@ -5,7 +5,8 @@ // constant folded away, or caught by the validator. // Ensure fmod is constant evaluated during codegen, or dxil const eval -// TODO: handle fp specials properly! +// TODO: handle fp specials properly, tracked with https://github.com/microsoft/DirectXShaderCompiler/issues/6567 + RWBuffer results : register(u0); @@ -29,6 +30,25 @@ void main(bool b : B) { fmod(5.5f, -3.0f), fmod(-5.5f, -3.0f)); +#ifdef SPECIALS + // Literal float + // 0.0, -0.0, NaN, -NaN + // SPECIALS: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 2, i32 undef, float 0.000000e+00, -0.000000e+00, float 0x7FF8000000000000, float 0x7FF8000000000000, i8 15) + results[i++] = float4(fmod(0.0, 1.0), + fmod(-0.0, 1.0), + fmod(5.5, 0.0), + fmod(-5.5, 0.0)); + + // Explicit float + // 0.0, -0.0, NaN, -NaN + // SPECIALS: call void @dx.op.bufferStore.f32(i32 69, %dx.types.Handle %{{.+}}, i32 3, i32 undef, float 0.000000e+00, -0.000000e+00, float 0x7FF8000000000000, float 0x7FF8000000000000, i8 15) + results[i++] = float4(fmod(0.0f, 1.0f), + fmod(-0.0f, 1.0f), + fmod(5.5f, 0.0f), + fmod(-5.5f, 0.0f)); + +#endif // SPECIALS + #ifdef NO_FOLD // Currently, we rely on constant folding of DXIL ops to get rid of illegal // double overloads. If this doesn't happen, we expect a validation error. diff --git a/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl index 9b60ae95cb..4e6fbec23f 100644 --- a/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl +++ b/tools/clang/test/CodeGenDXIL/literal/length_const_eval.hlsl @@ -5,7 +5,7 @@ // constant folded away, or caught by the validator. // Ensure length is constant evaluated during codegen, or dxil const eval -// TODO: handle fp specials properly! +// TODO: handle fp specials properly, tracked with https://github.com/microsoft/DirectXShaderCompiler/issues/6567 RWBuffer results : register(u0); diff --git a/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl b/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl index b8d14d3fcc..280bd30465 100644 --- a/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl +++ b/tools/clang/test/CodeGenDXIL/literal/normalize_const_eval.hlsl @@ -5,7 +5,8 @@ // constant folded away, or caught by the validator. // Ensure normalize is constant evaluated during codegen, or dxil const eval -// TODO: handle fp specials properly! +// TODO: handle fp specials properly, tracked with https://github.com/microsoft/DirectXShaderCompiler/issues/6567 + RWBuffer results : register(u0);