From da0396d75604d17a2e49ab41186b4a91865d7242 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Wed, 6 Dec 2023 11:59:57 -0800 Subject: [PATCH 1/8] Do some basic validation of Target Features (#7986) --- src/Target.cpp | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/Target.h | 6 ++++ 2 files changed, 98 insertions(+) diff --git a/src/Target.cpp b/src/Target.cpp index 597d5bf5367d..2d638dcacd67 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -788,6 +788,98 @@ void bad_target_string(const std::string &target) { } // namespace +void Target::validate_features() const { + // Note that the features don't have to be exhaustive, but enough to avoid obvious mistakes is good. + if (arch == X86) { + static const std::vector bad_features_x86 = { + ARMDotProd, + ARMFp16, + ARMv7s, + ARMv81a, + HVX_128, + HVX_128, + HVX_v62, + HVX_v65, + HVX_v66, + NoNEON, + POWER_ARCH_2_07, + RVV, + SVE, + SVE2, + VSX, + WasmBulkMemory, + WasmSatFloatToInt, + WasmSignExt, + WasmSimd128, + WasmThreads, + }; + user_assert(!features_any_of(bad_features_x86)) << "At least os_entry of the features for " + << *this << " is incompatible with the Target's architecture."; + } else if (arch == ARM) { + static const std::vector bad_features_arm = { + AVX, + AVX2, + AVX512, + AVX512_Cannonlake, + AVX512_KNL, + AVX512_SapphireRapids, + AVX512_Skylake, + AVX512_Zen4, + F16C, + FMA, + FMA4, + HVX_128, + HVX_128, + HVX_v62, + HVX_v65, + HVX_v66, + POWER_ARCH_2_07, + RVV, + SSE41, + VSX, + WasmBulkMemory, + WasmSatFloatToInt, + WasmSignExt, + WasmSimd128, + WasmThreads, + }; + user_assert(!features_any_of(bad_features_arm)) << "At least os_entry of the features for " + << *this << " is incompatible with the Target's architecture."; + } else if (arch == WebAssembly) { + static const std::vector bad_features_wasm = { + ARMDotProd, + ARMFp16, + ARMv7s, + ARMv81a, + AVX, + AVX2, + AVX512, + AVX512_Cannonlake, + AVX512_KNL, + AVX512_SapphireRapids, + AVX512_Skylake, + AVX512_Zen4, + F16C, + FMA, + FMA4, + HVX_128, + HVX_128, + HVX_v62, + HVX_v65, + HVX_v66, + NoNEON, + POWER_ARCH_2_07, + RVV, + SSE41, + SVE, + SVE2, + VSX, + }; + user_assert(!features_any_of(bad_features_wasm)) << "At least os_entry of the features for " + << *this << " is incompatible with the Target's architecture."; + } +} + Target::Target(const std::string &target) { Target host = get_host_target(); diff --git a/src/Target.h b/src/Target.h index 76b06aed6b8e..683c9c678d3b 100644 --- a/src/Target.h +++ b/src/Target.h @@ -178,6 +178,7 @@ struct Target { for (const auto &f : initial_features) { set_feature(f); } + validate_features(); } Target(OS o, Arch a, int b, const std::vector &initial_features = std::vector()) @@ -358,6 +359,11 @@ struct Target { private: /** A bitmask that stores the active features. */ std::bitset features; + + /** Attempt to validate that all features set are sensible for the base Target. + * This is *not* guaranteed to get all invalid combinations, but is intended + * to catch at least the most common (e.g., setting arm-specific features on x86). */ + void validate_features() const; }; /** Return the target corresponding to the host machine. */ From ed80bc91b1eb5f54663284fa6157f9a7b2bee17c Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Wed, 6 Dec 2023 12:03:14 -0800 Subject: [PATCH 2/8] Update Target.cpp --- src/Target.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Target.cpp b/src/Target.cpp index 2d638dcacd67..df5c03b9699d 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -813,7 +813,7 @@ void Target::validate_features() const { WasmSimd128, WasmThreads, }; - user_assert(!features_any_of(bad_features_x86)) << "At least os_entry of the features for " + user_assert(!features_any_of(bad_features_x86)) << "At least one of the features for " << *this << " is incompatible with the Target's architecture."; } else if (arch == ARM) { static const std::vector bad_features_arm = { @@ -843,7 +843,7 @@ void Target::validate_features() const { WasmSimd128, WasmThreads, }; - user_assert(!features_any_of(bad_features_arm)) << "At least os_entry of the features for " + user_assert(!features_any_of(bad_features_arm)) << "At least one of the features for " << *this << " is incompatible with the Target's architecture."; } else if (arch == WebAssembly) { static const std::vector bad_features_wasm = { @@ -875,7 +875,7 @@ void Target::validate_features() const { SVE2, VSX, }; - user_assert(!features_any_of(bad_features_wasm)) << "At least os_entry of the features for " + user_assert(!features_any_of(bad_features_wasm)) << "At least one of the features for " << *this << " is incompatible with the Target's architecture."; } } @@ -891,6 +891,7 @@ Target::Target(const std::string &target) { bad_target_string(target); } } + validate_features(); } Target::Target(const char *s) From 58fdc54b6c55e1eff47604036c0271d38fbc3ff4 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Wed, 6 Dec 2023 12:06:08 -0800 Subject: [PATCH 3/8] Update Target.cpp --- src/Target.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Target.cpp b/src/Target.cpp index df5c03b9699d..a5f0687ce0db 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -876,7 +876,7 @@ void Target::validate_features() const { VSX, }; user_assert(!features_any_of(bad_features_wasm)) << "At least one of the features for " - << *this << " is incompatible with the Target's architecture."; + << *this << " is incompatible with the Target's architecture."; } } From f619ad4727631e960770f745fa5d2dbbddba161d Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Wed, 6 Dec 2023 14:41:19 -0800 Subject: [PATCH 4/8] Fixes --- python_bindings/test/correctness/target.py | 5 +---- src/Target.cpp | 5 ----- test/correctness/target.cpp | 7 +++---- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/python_bindings/test/correctness/target.py b/python_bindings/test/correctness/target.py index 18eee2651301..7876bc97ecef 100644 --- a/python_bindings/test/correctness/target.py +++ b/python_bindings/test/correctness/target.py @@ -50,9 +50,6 @@ def test_target(): 32, [ hl.TargetFeature.JIT, - hl.TargetFeature.SSE41, - hl.TargetFeature.AVX, - hl.TargetFeature.AVX2, hl.TargetFeature.CUDA, hl.TargetFeature.OpenCL, hl.TargetFeature.OpenGLCompute, @@ -60,7 +57,7 @@ def test_target(): ], ) ts = t1.to_string() - assert ts == "arm-32-android-avx-avx2-cuda-debug-jit-opencl-openglcompute-sse41" + assert ts == "arm-32-android-cuda-debug-jit-opencl-openglcompute" assert hl.Target.validate_target_string(ts) # Expected failures: diff --git a/src/Target.cpp b/src/Target.cpp index a5f0687ce0db..6cf595362b3d 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -828,11 +828,6 @@ void Target::validate_features() const { F16C, FMA, FMA4, - HVX_128, - HVX_128, - HVX_v62, - HVX_v65, - HVX_v66, POWER_ARCH_2_07, RVV, SSE41, diff --git a/test/correctness/target.cpp b/test/correctness/target.cpp index 160d870ac09a..8fc03b589a73 100644 --- a/test/correctness/target.cpp +++ b/test/correctness/target.cpp @@ -51,11 +51,10 @@ int main(int argc, char **argv) { // Full specification round-trip, crazy features t1 = Target(Target::Android, Target::ARM, 32, - {Target::JIT, Target::SSE41, Target::AVX, Target::AVX2, - Target::CUDA, Target::OpenCL, Target::OpenGLCompute, - Target::Debug}); + {Target::JIT, Target::CUDA, Target::OpenCL, + Target::OpenGLCompute, Target::Debug}); ts = t1.to_string(); - if (ts != "arm-32-android-avx-avx2-cuda-debug-jit-opencl-openglcompute-sse41") { + if (ts != "arm-32-android-cuda-debug-jit-opencl-openglcompute") { printf("to_string failure: %s\n", ts.c_str()); return 1; } From ceefb6fe897e094188974c4d38fdc719f23993d6 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 7 Dec 2023 08:03:33 -0800 Subject: [PATCH 5/8] Update Target.cpp --- src/Target.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/Target.cpp b/src/Target.cpp index 6cf595362b3d..5580189999e5 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -796,11 +796,6 @@ void Target::validate_features() const { ARMFp16, ARMv7s, ARMv81a, - HVX_128, - HVX_128, - HVX_v62, - HVX_v65, - HVX_v66, NoNEON, POWER_ARCH_2_07, RVV, From 5dce0741879b39f6daf532666e105e991c2d6290 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 7 Dec 2023 16:40:09 -0800 Subject: [PATCH 6/8] Improve error messaging. --- src/Target.cpp | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/Target.cpp b/src/Target.cpp index 5580189999e5..daecb7068a35 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -786,12 +786,20 @@ void bad_target_string(const std::string &target) { << "On this platform, the host target is: " << get_host_target().to_string() << "\n"; } +void do_check_bad(const Target &t, const std::initializer_list &v) { + for (Target::Feature f : v) { + user_assert(!t.has_feature(f)) + << "Target feature " << Target::feature_to_name(f) + << " is incompatible with the Target's architecture. (" << t << ")\n"; + } +} + } // namespace void Target::validate_features() const { // Note that the features don't have to be exhaustive, but enough to avoid obvious mistakes is good. if (arch == X86) { - static const std::vector bad_features_x86 = { + do_check_bad(*this, { ARMDotProd, ARMFp16, ARMv7s, @@ -807,11 +815,9 @@ void Target::validate_features() const { WasmSignExt, WasmSimd128, WasmThreads, - }; - user_assert(!features_any_of(bad_features_x86)) << "At least one of the features for " - << *this << " is incompatible with the Target's architecture."; + }); } else if (arch == ARM) { - static const std::vector bad_features_arm = { + do_check_bad(*this, { AVX, AVX2, AVX512, @@ -832,11 +838,9 @@ void Target::validate_features() const { WasmSignExt, WasmSimd128, WasmThreads, - }; - user_assert(!features_any_of(bad_features_arm)) << "At least one of the features for " - << *this << " is incompatible with the Target's architecture."; + }); } else if (arch == WebAssembly) { - static const std::vector bad_features_wasm = { + do_check_bad(*this, { ARMDotProd, ARMFp16, ARMv7s, @@ -864,9 +868,7 @@ void Target::validate_features() const { SVE, SVE2, VSX, - }; - user_assert(!features_any_of(bad_features_wasm)) << "At least one of the features for " - << *this << " is incompatible with the Target's architecture."; + }); } } From fe4a6f24933bd550e4b9717097f8ce2dbd9ba753 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 7 Dec 2023 16:50:48 -0800 Subject: [PATCH 7/8] format --- src/Target.cpp | 134 ++++++++++++++++++++++++------------------------- 1 file changed, 67 insertions(+), 67 deletions(-) diff --git a/src/Target.cpp b/src/Target.cpp index daecb7068a35..d307bcb8be93 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -787,8 +787,8 @@ void bad_target_string(const std::string &target) { } void do_check_bad(const Target &t, const std::initializer_list &v) { - for (Target::Feature f : v) { - user_assert(!t.has_feature(f)) + for (Target::Feature f : v) { + user_assert(!t.has_feature(f)) << "Target feature " << Target::feature_to_name(f) << " is incompatible with the Target's architecture. (" << t << ")\n"; } @@ -800,75 +800,75 @@ void Target::validate_features() const { // Note that the features don't have to be exhaustive, but enough to avoid obvious mistakes is good. if (arch == X86) { do_check_bad(*this, { - ARMDotProd, - ARMFp16, - ARMv7s, - ARMv81a, - NoNEON, - POWER_ARCH_2_07, - RVV, - SVE, - SVE2, - VSX, - WasmBulkMemory, - WasmSatFloatToInt, - WasmSignExt, - WasmSimd128, - WasmThreads, - }); + ARMDotProd, + ARMFp16, + ARMv7s, + ARMv81a, + NoNEON, + POWER_ARCH_2_07, + RVV, + SVE, + SVE2, + VSX, + WasmBulkMemory, + WasmSatFloatToInt, + WasmSignExt, + WasmSimd128, + WasmThreads, + }); } else if (arch == ARM) { do_check_bad(*this, { - AVX, - AVX2, - AVX512, - AVX512_Cannonlake, - AVX512_KNL, - AVX512_SapphireRapids, - AVX512_Skylake, - AVX512_Zen4, - F16C, - FMA, - FMA4, - POWER_ARCH_2_07, - RVV, - SSE41, - VSX, - WasmBulkMemory, - WasmSatFloatToInt, - WasmSignExt, - WasmSimd128, - WasmThreads, - }); + AVX, + AVX2, + AVX512, + AVX512_Cannonlake, + AVX512_KNL, + AVX512_SapphireRapids, + AVX512_Skylake, + AVX512_Zen4, + F16C, + FMA, + FMA4, + POWER_ARCH_2_07, + RVV, + SSE41, + VSX, + WasmBulkMemory, + WasmSatFloatToInt, + WasmSignExt, + WasmSimd128, + WasmThreads, + }); } else if (arch == WebAssembly) { do_check_bad(*this, { - ARMDotProd, - ARMFp16, - ARMv7s, - ARMv81a, - AVX, - AVX2, - AVX512, - AVX512_Cannonlake, - AVX512_KNL, - AVX512_SapphireRapids, - AVX512_Skylake, - AVX512_Zen4, - F16C, - FMA, - FMA4, - HVX_128, - HVX_128, - HVX_v62, - HVX_v65, - HVX_v66, - NoNEON, - POWER_ARCH_2_07, - RVV, - SSE41, - SVE, - SVE2, - VSX, - }); + ARMDotProd, + ARMFp16, + ARMv7s, + ARMv81a, + AVX, + AVX2, + AVX512, + AVX512_Cannonlake, + AVX512_KNL, + AVX512_SapphireRapids, + AVX512_Skylake, + AVX512_Zen4, + F16C, + FMA, + FMA4, + HVX_128, + HVX_128, + HVX_v62, + HVX_v65, + HVX_v66, + NoNEON, + POWER_ARCH_2_07, + RVV, + SSE41, + SVE, + SVE2, + VSX, + }); } } From 2ad9fe0397ed76e16cf72d6d3224df09e040c991 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Fri, 8 Dec 2023 09:07:26 -0800 Subject: [PATCH 8/8] Update Target.cpp --- src/Target.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Target.cpp b/src/Target.cpp index 6505e889c29f..49011348544f 100644 --- a/src/Target.cpp +++ b/src/Target.cpp @@ -810,8 +810,7 @@ void Target::validate_features() const { SVE2, VSX, WasmBulkMemory, - WasmSatFloatToInt, - WasmSignExt, + WasmMvpOnly, WasmSimd128, WasmThreads, }); @@ -833,8 +832,7 @@ void Target::validate_features() const { SSE41, VSX, WasmBulkMemory, - WasmSatFloatToInt, - WasmSignExt, + WasmMvpOnly, WasmSimd128, WasmThreads, });