Hi Akira, I don't think there's currently a good reason we don't perform a diagnostic there, just right now we'd have to say something bland like "unable to inline always_inline function 'foo'" without a reason. We might not have gotten a diagnostic in the past because of compatibility reasons/transition reasons, but even gcc warns here. Logically the programmer said "always_inline" - if we can't we should probably say something.
-eric On Wed, Nov 11, 2015 at 6:17 PM Akira Hatanaka <ahata...@gmail.com> wrote: > Currently, we inline a function only if the call to isInlineViable returns > true, which means there are cases where we don't inline functions marked > always_inline. Is there a reason we haven't made changes to produce any > diagnostic in those cases? The comment also says "should be inlined > whenever possible", which gives the impression that this is only a best > effort attribute. > > > On Wed, Nov 11, 2015 at 6:09 PM, Eric Christopher <echri...@gmail.com> > wrote: > >> >> >> On Wed, Nov 11, 2015 at 6:08 PM Akira Hatanaka <ahata...@gmail.com> >> wrote: >> >>> I think you are suggesting we change the inliner to produce a diagnostic >>> (error or warning?) when the callee is marked always_inline and its >>> function attributes are not compatible with the caller's >>> (functionsHaveCompatibleAttributes returns false). Is that correct? >>> >>> >> Yep. And I think an error is appropriate here. The programmer said that >> the function must always inline and it's not always being inlined. >> >> I think it's a larger change because it'll require some refactoring and >> the ability to give reasons out of the always inline pass when faced with >> cost analysis. >> >> -eric >> >> >>> On Wed, Nov 11, 2015 at 4:48 PM, Eric Christopher <echri...@gmail.com> >>> wrote: >>> >>>> FWIW we should also have the backend avoid inlining and perhaps produce >>>> a diagnostic if something makes it there as well. >>>> >>>> -eric >>>> >>>> On Wed, Nov 11, 2015 at 4:46 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">; >>>>> 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