Author: Chi-Chun, Chen Date: 2026-10-22T01:52:25-05:00 New Revision: 3c71e661b96c89a82338163128cc9be4578f7f1a
URL: https://github.com/llvm/llvm-project/commit/3c71e661b96c89a82338163128cc9be4578f7f1a DIFF: https://github.com/llvm/llvm-project/commit/3c71e661b96c89a82338163128cc9be4578f7f1a.diff LOG: [clang][OpenMP] Match device and target_device ISA names separately (#232996) A context selector can name ISAs in both `device={isa(...)}` and `target_device={isa(...)}`. These are separate selectors, but their ISA names were checked together, so an ISA that the target does not support made both selectors fail. The default match_all needs both selectors anyway, but the `match_any` and `match_none` extensions selected the wrong variant. For example, on x86-64: ```c #pragma omp declare variant(variant) \ match(device = {isa(sse2)}, target_device = {isa(bogus_isa)}, \ implementation = {extension(match_any)}) ``` did not select variant, although `device={isa(sse2)}` matches. Likewise, `match_none` accepted a variant whose `target_device` ISA matched. This patch records which selector each ISA name belongs to and check each selector against its own names only. Because begin declare variant only looks at the device and implementation sets, a `target_device` ISA name no longer causes a begin declare variant block to be elided. Fixes https://github.com/llvm/llvm-project/issues/232990. Assisted-by: GitHub Copilot (Claude Opus 5.5) Added: clang/test/OpenMP/declare_variant_isa_selectors.c Modified: llvm/include/llvm/Frontend/OpenMP/OMPContext.h llvm/lib/Frontend/OpenMP/OMPContext.cpp llvm/unittests/Frontend/OpenMPContextTest.cpp Removed: ################################################################################ diff --git a/clang/test/OpenMP/declare_variant_isa_selectors.c b/clang/test/OpenMP/declare_variant_isa_selectors.c new file mode 100644 index 0000000000000..0b022638a538a --- /dev/null +++ b/clang/test/OpenMP/declare_variant_isa_selectors.c @@ -0,0 +1,35 @@ +// RUN: %clang_cc1 -fopenmp -triple x86_64-unknown-linux -emit-llvm %s \ +// RUN: -Wno-source-uses-openmp -o - | FileCheck %s + +// Device and target_device ISA selectors are matched independently. + +int variant(void); + +// MATCH_ANY is satisfied by the supported device ISA. +#pragma omp declare variant(variant) \ + match(device = {isa(sse2)}, target_device = {isa(bogus_isa)}, \ + implementation = {extension(match_any)}) +int any_base(void); + +// CHECK-LABEL: define{{.*}} i32 @any_caller( +// CHECK: call i32 @variant() +int any_caller(void) { return any_base(); } + +// MATCH_NONE is not satisfied because the target_device ISA is supported. +#pragma omp declare variant(variant) \ + match(device = {isa(bogus_isa)}, target_device = {isa(sse2)}, \ + implementation = {extension(match_none)}) +int none_base(void); + +// CHECK-LABEL: define{{.*}} i32 @none_caller( +// CHECK: call i32 @none_base() +int none_caller(void) { return none_base(); } + +// BEGIN DECLARE VARIANT ignores target_device traits, so the unsupported +// target_device ISA does not elide the block. +int mixed(void) { return 0; } +#pragma omp begin declare variant \ + match(device = {isa(sse2)}, target_device = {isa(bogus_isa)}) +// CHECK: define{{.*}} i32 @"mixed$ompvariant${{.*}}"() +int mixed(void) { return 1; } +#pragma omp end declare variant diff --git a/llvm/include/llvm/Frontend/OpenMP/OMPContext.h b/llvm/include/llvm/Frontend/OpenMP/OMPContext.h index 00837c661b54d..1231fa1b9c1ca 100644 --- a/llvm/include/llvm/Frontend/OpenMP/OMPContext.h +++ b/llvm/include/llvm/Frontend/OpenMP/OMPContext.h @@ -122,6 +122,15 @@ LLVM_ABI bool isValidTraitPropertyForTraitSetAndSelector(TraitProperty Property, /// scored (via the ScoresMap). In addition, the required consturct nesting is /// decribed as well. struct VariantMatchInfo { + /// A device or target_device isa property and the ISA name it requires. + struct ISATrait { + TraitProperty Property; + StringRef Name; + bool operator==(const ISATrait &Other) const { + return Property == Other.Property && Name == Other.Name; + } + }; + /// Add the trait \p Property to the required trait set. \p RawString is the /// string we parsed and derived \p Property from. If \p Score is not null, it /// recorded as well. If \p Property is in the `construct` set it is recorded @@ -142,10 +151,9 @@ struct VariantMatchInfo { // Special handling for `device={isa(...)}` as we do not match the enum but // the raw string. - if (Property == TraitProperty::device_isa___ANY) - ISATraits.push_back(RawString); - if (Property == TraitProperty::target_device_isa___ANY) - ISATraits.push_back(RawString); + if (Property == TraitProperty::device_isa___ANY || + Property == TraitProperty::target_device_isa___ANY) + ISATraits.push_back({Property, RawString}); RequiredTraits.set(unsigned(Property)); if (Set == TraitSet::construct) @@ -153,7 +161,7 @@ struct VariantMatchInfo { } BitVector RequiredTraits = BitVector(unsigned(TraitProperty::Last) + 1); - SmallVector<StringRef, 8> ISATraits; + SmallVector<ISATrait, 8> ISATraits; SmallVector<TraitProperty, 8> ConstructTraits; SmallDenseMap<TraitProperty, APInt> ScoreMap; }; diff --git a/llvm/lib/Frontend/OpenMP/OMPContext.cpp b/llvm/lib/Frontend/OpenMP/OMPContext.cpp index e77e48913fdd0..c7b689aee50ef 100644 --- a/llvm/lib/Frontend/OpenMP/OMPContext.cpp +++ b/llvm/lib/Frontend/OpenMP/OMPContext.cpp @@ -182,9 +182,10 @@ static bool isStrictSubset(const VariantMatchInfo &VMI0, TraitProperty::user_condition_unknown}) if (VMI0.RequiredTraits.test(unsigned(Property))) return false; - return llvm::all_of(VMI0.ISATraits, [&](StringRef Name) { - return llvm::is_contained(VMI1.ISATraits, Name); - }); + return llvm::all_of(VMI0.ISATraits, + [&](const VariantMatchInfo::ISATrait &Trait) { + return llvm::is_contained(VMI1.ISATraits, Trait); + }); } static int @@ -257,13 +258,10 @@ isVariantApplicableInContextHelper(const VariantMatchInfo &VMI, // We overwrite the isa trait as it is actually up to the OMPContext hook to // check the raw string(s). - if (Property == TraitProperty::device_isa___ANY) - IsActiveTrait = llvm::all_of(VMI.ISATraits, [&](StringRef RawString) { - return Ctx.matchesISATrait(RawString); - }); - if (Property == TraitProperty::target_device_isa___ANY) - IsActiveTrait = llvm::all_of(VMI.ISATraits, [&](StringRef RawString) { - return Ctx.matchesISATrait(RawString); + if (Property == TraitProperty::device_isa___ANY || + Property == TraitProperty::target_device_isa___ANY) + IsActiveTrait = llvm::all_of(VMI.ISATraits, [&](const auto &Trait) { + return Trait.Property != Property || Ctx.matchesISATrait(Trait.Name); }); if (std::optional<bool> Result = HandleTrait(Property, IsActiveTrait)) diff --git a/llvm/unittests/Frontend/OpenMPContextTest.cpp b/llvm/unittests/Frontend/OpenMPContextTest.cpp index ddba513e5330c..5227229a56173 100644 --- a/llvm/unittests/Frontend/OpenMPContextTest.cpp +++ b/llvm/unittests/Frontend/OpenMPContextTest.cpp @@ -313,6 +313,38 @@ TEST_F(OpenMPContextTest, ApplicabilityAllTraits) { } } +TEST_F(OpenMPContextTest, ApplicabilityISATraits) { + struct SSE2Context : OMPContext { + using OMPContext::OMPContext; + bool matchesISATrait(StringRef ISA) const override { return ISA == "sse2"; } + }; + SSE2Context Context(false, Triple("x86_64-unknown-linux"), Triple(), -1); + + // An unsupported ISA in one selector must not fail the other selector. + for (auto [Supported, Unsupported] : + {std::pair(TraitProperty::device_isa___ANY, + TraitProperty::target_device_isa___ANY), + std::pair(TraitProperty::target_device_isa___ANY, + TraitProperty::device_isa___ANY)}) { + VariantMatchInfo VMI; + VMI.addTrait(Supported, "sse2"); + VMI.addTrait(Unsupported, "bogus_isa"); + EXPECT_FALSE(isVariantApplicableInContext(VMI, Context)); + VariantMatchInfo Any = VMI, None = VMI; + Any.addTrait(TraitProperty::implementation_extension_match_any, ""); + EXPECT_TRUE(isVariantApplicableInContext(Any, Context)); + None.addTrait(TraitProperty::implementation_extension_match_none, ""); + EXPECT_FALSE(isVariantApplicableInContext(None, Context)); + } + + // Every name in one selector is still required. + VariantMatchInfo TwoNames; + TwoNames.addTrait(TraitProperty::device_isa___ANY, "sse2"); + TwoNames.addTrait(TraitProperty::device_isa___ANY, "bogus_isa"); + TwoNames.addTrait(TraitProperty::implementation_extension_match_any, ""); + EXPECT_FALSE(isVariantApplicableInContext(TwoNames, Context)); +} + TEST_F(OpenMPContextTest, ScoringSimple) { // TODO: Add scoring tests (via getBestVariantMatchForContext). } @@ -391,7 +423,7 @@ TEST_F(OpenMPContextTest, RepeatedConstructWithConditionOrISA) { // Each pair ties and requires the same traits, but the conditions or ISA // names may diff er, so clause order decides. - APInt Four(64, 4), Twenty(64, 20); + APInt Four(64, 4), Twenty(64, 20), ThirtySix(64, 36); for (auto [Property, Score] : {std::pair(TraitProperty::user_condition_true, &Four), std::pair(TraitProperty::device_isa___ANY, &Twenty)}) { @@ -419,6 +451,17 @@ TEST_F(OpenMPContextTest, RepeatedConstructWithConditionOrISA) { EXPECT_EQ(getBestVariantMatchForContext(Candidates, Context), 0); std::swap(Candidates[0], Candidates[1]); EXPECT_EQ(getBestVariantMatchForContext(Candidates, Context), 1); + + // The same ISA names in swapped selectors are not subsets of each other. + ParallelFor.addTrait(TraitProperty::implementation_vendor_llvm, "", + &ThirtySix); + ParallelFor.addTrait(TraitProperty::target_device_isa___ANY, "sse2"); + ParallelForParallel.addTrait(TraitProperty::target_device_isa___ANY, + "sse"); + Candidates = {ParallelFor, ParallelForParallel}; + EXPECT_EQ(getBestVariantMatchForContext(Candidates, Context), 0); + std::swap(Candidates[0], Candidates[1]); + EXPECT_EQ(getBestVariantMatchForContext(Candidates, Context), 0); } } } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
