https://github.com/chichunchen updated https://github.com/llvm/llvm-project/pull/232996
>From d8fba99cf6bb56f1a1ef7d5afdc78b546d9233cc Mon Sep 17 00:00:00 2001 From: "Chi Chun, Chen" <[email protected]> Date: Fri, 16 Oct 2026 14:19:51 -0500 Subject: [PATCH] [clang][OpenMP] Match device and target_device ISA names separately 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: #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) --- .../OpenMP/declare_variant_isa_selectors.c | 35 +++++++++++++++ .../include/llvm/Frontend/OpenMP/OMPContext.h | 18 +++++--- llvm/lib/Frontend/OpenMP/OMPContext.cpp | 18 ++++---- llvm/unittests/Frontend/OpenMPContextTest.cpp | 45 ++++++++++++++++++- 4 files changed, 100 insertions(+), 16 deletions(-) create mode 100644 clang/test/OpenMP/declare_variant_isa_selectors.c 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 00000000000000..0b022638a538a2 --- /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 00837c661b54db..1231fa1b9c1ca8 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 e77e48913fdd0c..c7b689aee50ef8 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 ddba513e5330cb..5227229a56173d 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 differ, 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
