On Wed, Nov 11, 2015 at 5:38 PM Richard Smith <rich...@metafoo.co.uk> wrote:
> On Wed, Nov 11, 2015 at 5:25 PM, Eric Christopher <echri...@gmail.com> > wrote: > >> On Wed, Nov 11, 2015 at 5:03 PM Richard Smith <rich...@metafoo.co.uk> >> wrote: >> >>> On Wed, Nov 11, 2015 at 4:44 PM, Eric Christopher via cfe-commits < >>> cfe-commits@lists.llvm.org> wrote: >>> >>>> Author: echristo >>>> Date: Wed Nov 11 18:44:12 2015 >>>> New Revision: 252834 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=252834&view=rev >>>> Log: >>>> Provide a frontend based error for always_inline functions that require >>>> target features that the caller function doesn't provide. This matches >>>> the existing backend failure to inline functions that don't have >>>> matching target features - and diagnoses earlier in the case of >>>> always_inline. >>>> >>>> Fix up a few test cases that were, in fact, invalid if you tried >>>> to generate code from the backend with the specified target features >>>> and add a couple of tests to illustrate what's going on. >>>> >>>> This should fix PR25246. >>>> >>>> Added: >>>> cfe/trunk/test/CodeGen/target-features-error-2.c >>>> cfe/trunk/test/CodeGen/target-features-error.c >>>> Modified: >>>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >>>> cfe/trunk/lib/CodeGen/CGExpr.cpp >>>> cfe/trunk/lib/CodeGen/CodeGenFunction.cpp >>>> cfe/trunk/test/CodeGen/3dnow-builtins.c >>>> cfe/trunk/test/CodeGen/avx512vl-builtins.c >>>> >>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=252834&r1=252833&r2=252834&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Nov 11 >>>> 18:44:12 2015 >>>> @@ -431,6 +431,9 @@ def err_builtin_definition : Error<"defi >>>> def err_arm_invalid_specialreg : Error<"invalid special register for >>>> builtin">; >>>> def err_invalid_cpu_supports : Error<"invalid cpu feature string for >>>> builtin">; >>>> def err_builtin_needs_feature : Error<"%0 needs target feature %1">; >>>> +def err_function_needs_feature >>>> + : Error<"function %0 and always_inline callee function %1 are >>>> required to " >>>> + "have matching target features">; >>>> >>> >>> It would be useful to say which feature is missing here. Also, >>> "matching" implies to >>> >> >> Right. >> >> >>> me that the sets must be the same, whereas I think the rule is that the >>> caller must at least all the features that the callee uses. Something like: >>> "always_inline callee function %0 uses target feature %1 that caller %2 >>> does not require" might be more useful? >>> >> >> So, I was waiting on this I admit :) >> >> What we really need is a "string set" way of doing set difference and >> then optionally translating that to command line options. I'm really leery >> of doing the "full set of options missing" though due to just the sheer >> size of the error messages out the other end. i.e. "default" compiles and >> "function that requires everything a skylake processor has". An >> intermediate result could be taking a look at the target attribute on the >> callee and posting that as a "set" of features that are required by the >> caller without bothering to do the set difference? >> >> I'll look into doing that, though it might not be immediately. >> >> Thoughts? >> > > In the short term, just listing the first feature you find that's missing > would help (the feature that causes you to fall out of the all_of check > below). > > Sure, that's reasonable. -eric > -eric >> >> >>> >>> >>>> def warn_builtin_unknown : Warning<"use of unknown builtin %0">, >>>> InGroup<ImplicitFunctionDeclare>, DefaultError; >>>> def warn_dyn_class_memaccess : Warning< >>>> >>>> Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=252834&r1=252833&r2=252834&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/CodeGen/CGExpr.cpp (original) >>>> +++ cfe/trunk/lib/CodeGen/CGExpr.cpp Wed Nov 11 18:44:12 2015 >>>> @@ -3747,6 +3747,15 @@ RValue CodeGenFunction::EmitCall(QualTyp >>>> assert(CalleeType->isFunctionPointerType() && >>>> "Call must have function pointer type!"); >>>> >>>> + if (const FunctionDecl *FD = >>>> dyn_cast_or_null<FunctionDecl>(TargetDecl)) >>>> + // If this isn't an always_inline function we can't guarantee that >>>> any >>>> + // function isn't being used correctly so only check if we have the >>>> + // attribute and a set of target attributes that might be >>>> different from >>>> + // our default. >>>> + if (TargetDecl->hasAttr<AlwaysInlineAttr>() && >>>> + TargetDecl->hasAttr<TargetAttr>()) >>>> + checkTargetFeatures(E, FD); >>>> + >>>> CalleeType = getContext().getCanonicalType(CalleeType); >>>> >>>> const auto *FnType = >>>> >>>> Modified: cfe/trunk/lib/CodeGen/CodeGenFunction.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenFunction.cpp?rev=252834&r1=252833&r2=252834&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/lib/CodeGen/CodeGenFunction.cpp (original) >>>> +++ cfe/trunk/lib/CodeGen/CodeGenFunction.cpp Wed Nov 11 18:44:12 2015 >>>> @@ -1843,7 +1843,8 @@ template void CGBuilderInserter<Preserve >>>> llvm::BasicBlock::iterator InsertPt) const; >>>> #undef PreserveNames >>>> >>>> -// Returns true if we have a valid set of target features. >>>> +// Emits an error if we don't have a valid set of target features for >>>> the >>>> +// called function. >>>> void CodeGenFunction::checkTargetFeatures(const CallExpr *E, >>>> const FunctionDecl >>>> *TargetDecl) { >>>> // Early exit if this is an indirect call. >>>> @@ -1856,31 +1857,70 @@ void CodeGenFunction::checkTargetFeature >>>> if (!FD) >>>> return; >>>> >>>> + // Grab the required features for the call. For a builtin this is >>>> listed in >>>> + // the td file with the default cpu, for an always_inline function >>>> this is any >>>> + // listed cpu and any listed features. >>>> unsigned BuiltinID = TargetDecl->getBuiltinID(); >>>> - const char *FeatureList = >>>> - CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID); >>>> - >>>> - if (!FeatureList || StringRef(FeatureList) == "") >>>> - return; >>>> - >>>> - llvm::StringMap<bool> FeatureMap; >>>> - CGM.getFunctionFeatureMap(FeatureMap, FD); >>>> - >>>> - // If we have at least one of the features in the feature list return >>>> - // true, otherwise return false. >>>> - SmallVector<StringRef, 1> AttrFeatures; >>>> - StringRef(FeatureList).split(AttrFeatures, ","); >>>> - if (!std::all_of(AttrFeatures.begin(), AttrFeatures.end(), >>>> - [&](StringRef &Feature) { >>>> - SmallVector<StringRef, 1> OrFeatures; >>>> - Feature.split(OrFeatures, "|"); >>>> - return std::any_of(OrFeatures.begin(), >>>> OrFeatures.end(), >>>> - [&](StringRef &Feature) { >>>> - return FeatureMap[Feature]; >>>> - }); >>>> - })) >>>> - CGM.getDiags().Report(E->getLocStart(), >>>> diag::err_builtin_needs_feature) >>>> - << TargetDecl->getDeclName() >>>> - << CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID); >>>> + if (BuiltinID) { >>>> + SmallVector<StringRef, 1> ReqFeatures; >>>> + const char *FeatureList = >>>> + CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID); >>>> + // Return if the builtin doesn't have any required features. >>>> + if (!FeatureList || StringRef(FeatureList) == "") >>>> + return; >>>> + StringRef(FeatureList).split(ReqFeatures, ","); >>>> + >>>> + // If there aren't any required features listed then go ahead and >>>> return. >>>> + if (ReqFeatures.empty()) >>>> + return; >>>> + >>>> + // Now build up the set of caller features and verify that all the >>>> required >>>> + // features are there. >>>> + llvm::StringMap<bool> CallerFeatureMap; >>>> + CGM.getFunctionFeatureMap(CallerFeatureMap, FD); >>>> + >>>> + // If we have at least one of the features in the feature list >>>> return >>>> + // true, otherwise return false. >>>> + if (!std::all_of( >>>> + ReqFeatures.begin(), ReqFeatures.end(), [&](StringRef >>>> &Feature) { >>>> + SmallVector<StringRef, 1> OrFeatures; >>>> + Feature.split(OrFeatures, "|"); >>>> + return std::any_of(OrFeatures.begin(), OrFeatures.end(), >>>> + [&](StringRef &Feature) { >>>> + return >>>> CallerFeatureMap.lookup(Feature); >>>> + }); >>>> + })) >>>> + CGM.getDiags().Report(E->getLocStart(), >>>> diag::err_builtin_needs_feature) >>>> + << TargetDecl->getDeclName() >>>> + << >>>> CGM.getContext().BuiltinInfo.getRequiredFeatures(BuiltinID); >>>> + >>>> + } else if (TargetDecl->hasAttr<TargetAttr>()) { >>>> + // Get the required features for the callee. >>>> + SmallVector<StringRef, 1> ReqFeatures; >>>> + llvm::StringMap<bool> CalleeFeatureMap; >>>> + CGM.getFunctionFeatureMap(CalleeFeatureMap, TargetDecl); >>>> + for (const auto &F : CalleeFeatureMap) >>>> + ReqFeatures.push_back(F.getKey()); >>>> + // If there aren't any required features listed then go ahead and >>>> return. >>>> + if (ReqFeatures.empty()) >>>> + return; >>>> + >>>> + // Now get the features that the caller provides. >>>> + llvm::StringMap<bool> CallerFeatureMap; >>>> + CGM.getFunctionFeatureMap(CallerFeatureMap, FD); >>>> + >>>> + // If we have at least one of the features in the feature list >>>> return >>>> + // true, otherwise return false. >>>> + if (!std::all_of( >>>> + ReqFeatures.begin(), ReqFeatures.end(), [&](StringRef >>>> &Feature) { >>>> + SmallVector<StringRef, 1> OrFeatures; >>>> + Feature.split(OrFeatures, "|"); >>>> + return std::any_of(OrFeatures.begin(), OrFeatures.end(), >>>> + [&](StringRef &Feature) { >>>> + return >>>> CallerFeatureMap.lookup(Feature); >>>> + }); >>>> + })) >>>> + CGM.getDiags().Report(E->getLocStart(), >>>> diag::err_function_needs_feature) >>>> + << FD->getDeclName() << TargetDecl->getDeclName(); >>>> + } >>>> } >>>> - >>>> >>>> Modified: cfe/trunk/test/CodeGen/3dnow-builtins.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/3dnow-builtins.c?rev=252834&r1=252833&r2=252834&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/CodeGen/3dnow-builtins.c (original) >>>> +++ cfe/trunk/test/CodeGen/3dnow-builtins.c Wed Nov 11 18:44:12 2015 >>>> @@ -1,6 +1,6 @@ >>>> // REQUIRES: x86-registered-target >>>> -// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -target-feature >>>> +3dnow -emit-llvm -o - -Werror | FileCheck %s >>>> -// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -target-feature >>>> +3dnow -S -o - -Werror | FileCheck %s --check-prefix=CHECK-ASM >>>> +// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -target-feature >>>> +3dnowa -emit-llvm -o - -Werror | FileCheck %s >>>> +// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -target-feature >>>> +3dnowa -S -o - -Werror | FileCheck %s --check-prefix=CHECK-ASM >>>> >>>> // Don't include mm_malloc.h, it's system specific. >>>> #define __MM_MALLOC_H >>>> >>>> Modified: cfe/trunk/test/CodeGen/avx512vl-builtins.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/avx512vl-builtins.c?rev=252834&r1=252833&r2=252834&view=diff >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/CodeGen/avx512vl-builtins.c (original) >>>> +++ cfe/trunk/test/CodeGen/avx512vl-builtins.c Wed Nov 11 18:44:12 2015 >>>> @@ -5,102 +5,6 @@ >>>> >>>> #include <immintrin.h> >>>> >>>> -__mmask8 test_mm256_cmpeq_epi32_mask(__m256i __a, __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_cmpeq_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.d.256 >>>> - return (__mmask8)_mm256_cmpeq_epi32_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_mask_cmpeq_epi32_mask(__mmask8 __u, __m256i __a, >>>> __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_mask_cmpeq_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.d.256 >>>> - return (__mmask8)_mm256_mask_cmpeq_epi32_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_cmpeq_epi32_mask(__m128i __a, __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_cmpeq_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.d.128 >>>> - return (__mmask8)_mm_cmpeq_epi32_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_mask_cmpeq_epi32_mask(__mmask8 __u, __m128i __a, >>>> __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_mask_cmpeq_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.d.128 >>>> - return (__mmask8)_mm_mask_cmpeq_epi32_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_cmpeq_epi64_mask(__m256i __a, __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_cmpeq_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.q.256 >>>> - return (__mmask8)_mm256_cmpeq_epi64_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_mask_cmpeq_epi64_mask(__mmask8 __u, __m256i __a, >>>> __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_mask_cmpeq_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.q.256 >>>> - return (__mmask8)_mm256_mask_cmpeq_epi64_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_cmpeq_epi64_mask(__m128i __a, __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_cmpeq_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.q.128 >>>> - return (__mmask8)_mm_cmpeq_epi64_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_mask_cmpeq_epi64_mask(__mmask8 __u, __m128i __a, >>>> __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_mask_cmpeq_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpeq.q.128 >>>> - return (__mmask8)_mm_mask_cmpeq_epi64_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_cmpgt_epi32_mask(__m256i __a, __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_cmpgt_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.d.256 >>>> - return (__mmask8)_mm256_cmpgt_epi32_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_mask_cmpgt_epi32_mask(__mmask8 __u, __m256i __a, >>>> __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_mask_cmpgt_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.d.256 >>>> - return (__mmask8)_mm256_mask_cmpgt_epi32_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_cmpgt_epi32_mask(__m128i __a, __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_cmpgt_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.d.128 >>>> - return (__mmask8)_mm_cmpgt_epi32_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_mask_cmpgt_epi32_mask(__mmask8 __u, __m128i __a, >>>> __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_mask_cmpgt_epi32_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.d.128 >>>> - return (__mmask8)_mm_mask_cmpgt_epi32_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_cmpgt_epi64_mask(__m256i __a, __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_cmpgt_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.q.256 >>>> - return (__mmask8)_mm256_cmpgt_epi64_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm256_mask_cmpgt_epi64_mask(__mmask8 __u, __m256i __a, >>>> __m256i __b) { >>>> - // CHECK-LABEL: @test_mm256_mask_cmpgt_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.q.256 >>>> - return (__mmask8)_mm256_mask_cmpgt_epi64_mask(__u, __a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_cmpgt_epi64_mask(__m128i __a, __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_cmpgt_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.q.128 >>>> - return (__mmask8)_mm_cmpgt_epi64_mask(__a, __b); >>>> -} >>>> - >>>> -__mmask8 test_mm_mask_cmpgt_epi64_mask(__mmask8 __u, __m128i __a, >>>> __m128i __b) { >>>> - // CHECK-LABEL: @test_mm_mask_cmpgt_epi64_mask >>>> - // CHECK: @llvm.x86.avx512.mask.pcmpgt.q.128 >>>> - return (__mmask8)_mm_mask_cmpgt_epi64_mask(__u, __a, __b); >>>> -} >>>> - >>>> __mmask8 test_mm_cmpeq_epu32_mask(__m128i __a, __m128i __b) { >>>> // CHECK-LABEL: @test_mm_cmpeq_epu32_mask >>>> // CHECK: @llvm.x86.avx512.mask.ucmp.d.128(<4 x i32> {{.*}}, <4 x >>>> i32> {{.*}}, i32 0, i8 -1) >>>> >>>> Added: cfe/trunk/test/CodeGen/target-features-error-2.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/target-features-error-2.c?rev=252834&view=auto >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/CodeGen/target-features-error-2.c (added) >>>> +++ cfe/trunk/test/CodeGen/target-features-error-2.c Wed Nov 11 >>>> 18:44:12 2015 >>>> @@ -0,0 +1,7 @@ >>>> +// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - >>>> +#define __MM_MALLOC_H >>>> +#include <x86intrin.h> >>>> + >>>> +int baz(__m256i a) { >>>> + return _mm256_extract_epi32(a, 3); // expected-error {{function >>>> 'baz' and always_inline callee function '_mm256_extract_epi32' are required >>>> to have matching target features}} >>>> +} >>>> >>>> Added: cfe/trunk/test/CodeGen/target-features-error.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/target-features-error.c?rev=252834&view=auto >>>> >>>> ============================================================================== >>>> --- cfe/trunk/test/CodeGen/target-features-error.c (added) >>>> +++ cfe/trunk/test/CodeGen/target-features-error.c Wed Nov 11 18:44:12 >>>> 2015 >>>> @@ -0,0 +1,8 @@ >>>> +// RUN: %clang_cc1 %s -triple=x86_64-linux-gnu -S -verify -o - >>>> +int __attribute__((target("avx"), always_inline)) foo(int a) { >>>> + return a + 4; >>>> +} >>>> +int bar() { >>>> + return foo(4); // expected-error {{function 'bar' and always_inline >>>> callee function 'foo' are required to have matching target features}} >>>> +} >>>> + >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> cfe-commits@lists.llvm.org >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >>>> >>>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
