Inlining function with mismatching target feature using [[gnu::flatten]] attribute causes fatal error

[mjacobse]

This minimal example leads to a fatal error (https://godbolt.org/z/Y6cM836Ts):

#include <smmintrin.h>

[[gnu::target("sse4.1")]]
__m128d test_impl() {
    return _mm_ceil_pd(_mm_set_sd(0.0));
}

[[gnu::flatten]]
__m128d test() {
    return test_impl();
}

fatal error: error in backend: Cannot select: 0x497142f0: v2f64 = X86ISD::VRNDSCALE 0x49714520, TargetConstant:i32<2>, example.c:5:12 @[ example.c:10:12 ]
  0x49714520: v2f64 = BUILD_VECTOR ConstantFP:f64<0.000000e+00>, ConstantFP:f64<0.000000e+00>, example.c:5:12 @[ example.c:10:12 ]
    0x49714440: f64 = ConstantFP<0.000000e+00>
    0x49714440: f64 = ConstantFP<0.000000e+00>
  0x49714210: i32 = TargetConstant<2>
In function: test

Using [[gnu::always_inline]] on test_impl instead of using [[gnu::flatten]] on test gives a helpful error message instead (https://godbolt.org/z/d5ebWj47n):

<source>:9:5: error: always_inline function 'test_impl' requires target feature 'sse4.1', but would be inlined into function 'test' that is compiled without support for 'sse4.1'
    9 |     test_impl();
      |     ^
1 error generated.
Compiler returned: 1

I am not entirely sure if the same helpful error message should be generated for the initial example, or if the target mismatch should overrule the flatten attribute instead, such that the function is simply not inlined. FWIW, gcc seems to do the 2nd option (https://godbolt.org/z/6Taf8qhzT). But either option seems preferable to the not so helpful fatal error.

[llvmbot]

@llvm/issue-subscribers-backend-x86

Author: Marcel Jacobse (mjacobse)

This minimal example leads to a fatal error (https://godbolt.org/z/Y6cM836Ts): ```c #include <smmintrin.h>

[[gnu::target("sse4.1")]]
__m128d test_impl() {
return _mm_ceil_pd(_mm_set_sd(0.0));
}

[[gnu::flatten]]
__m128d test() {
return test_impl();
}

fatal error: error in backend: Cannot select: 0x497142f0: v2f64 = X86ISD::VRNDSCALE 0x49714520, TargetConstant:i32<2>, example.c:5:12 @[ example.c:10:12 ]
0x49714520: v2f64 = BUILD_VECTOR ConstantFP:f64<0.000000e+00>, ConstantFP:f64<0.000000e+00>, example.c:5:12 @[ example.c:10:12 ]
0x49714440: f64 = ConstantFP<0.000000e+00>
0x49714440: f64 = ConstantFP<0.000000e+00>
0x49714210: i32 = TargetConstant<2>
In function: test

Using `[[gnu::always_inline]]` on `test_impl` instead of using `[[gnu::flatten]]` on `test` gives a helpful error message instead (https://godbolt.org/z/d5ebWj47n):

<source>:9:5: error: always_inline function 'test_impl' requires target feature 'sse4.1', but would be inlined into function 'test' that is compiled without support for 'sse4.1'
9 | test_impl();
| ^
1 error generated.
Compiler returned: 1

I am not entirely sure if the same helpful error message should be generated for the initial example, or if the target mismatch should overrule the flatten attribute instead, such that the function is simply not inlined. FWIW, gcc seems to do the 2nd option (https://godbolt.org/z/6Taf8qhzT). But either option seems preferable to the not so helpful fatal error.
</details>

[mjacobse]

Note that this is not specific to x86. For this ARMv8 example I get a similar fatal error (https://godbolt.org/z/3W8eavWGn)

#include <arm_neon.h>
#include <stdint.h>

[[gnu::target("+sha2")]]
uint32_t test_impl(uint32_t x) {
    return vsha1h_u32(x);
}

[[gnu::flatten]]
uint32_t test(uint32_t x) {
    return test_impl(x);
}

while [[gnu::always_inline]] instead of [[gnu::flatten]] leads to a nice error message (https://godbolt.org/z/91Tj9vrxz).

[abhishek-kaushik22]

The AST for the gnu::always_inline attribute is

TranslationUnitDecl
|-FunctionDecl <line:4:1, line:6:1> line:4:9 used test_impl '__m128d ()'
...
| `-AlwaysInlineAttr <col:26, col:31> always_inline
`-FunctionDecl <line:8:1, line:10:1> line:8:9 test '__m128d ()'
  `-CompoundStmt <col:16, line:10:1>
    `-ReturnStmt <line:9:5, col:22>
      `-CallExpr <col:12, col:22> '__m128d':'__attribute__((__vector_size__(2 * sizeof(double)))) double'
        `-ImplicitCastExpr <col:12> '__m128d (*)()' <FunctionToPointerDecay>
          `-DeclRefExpr <col:12> '__m128d ()' Function 0x1aa438a0 'test_impl' '__m128d ()'

slightly different than the gnu::flatten one

`-FunctionDecl <line:9:1, line:11:1> line:9:9 test '__m128d ()'
  |-CompoundStmt <col:16, line:11:1>
  | `-ReturnStmt <line:10:5, col:22>
  |   `-CallExpr <col:12, col:22> '__m128d':'__attribute__((__vector_size__(2 * sizeof(double)))) double'
  |     `-ImplicitCastExpr <col:12> '__m128d (*)()' <FunctionToPointerDecay>
  |       `-DeclRefExpr <col:12> '__m128d ()' Function 0x42cd71d0 'test_impl' '__m128d ()'
  `-FlattenAttr <line:8:3, col:8>

In the first case the attribute always inline is on the callee, and it's on the caller for the later.
The frontend is not checking for the second case, that's why we get the fatal error as the IR always-inliner pass doesn't check for target attributes.

  // CodeGenFunction::EmitCall
  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl)) {
    // We can only guarantee that a function is called from the correct
    // context/function based on the appropriate target attributes,
    // so only check in the case where we have both always_inline and target
    // since otherwise we could be making a conditional call after a check for
    // the proper cpu features (and it won't cause code generation issues due to
    // function based code generation).
    if (TargetDecl->hasAttr<AlwaysInlineAttr>() &&
        (TargetDecl->hasAttr<TargetAttr>() ||
         (CurFuncDecl && CurFuncDecl->hasAttr<TargetAttr>())))
      checkTargetFeatures(Loc, FD);
  }

Maybe a check for FlattenAttr can be added here or a similar check can be added to the IR inliner pass (or both)?
Also, silly question but why can't we just copy the extra target attributes from the function we are inlining?

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Marcel Jacobse (mjacobse)

This minimal example leads to a fatal error (https://godbolt.org/z/Y6cM836Ts): ```c #include <smmintrin.h>

[[gnu::target("sse4.1")]]
__m128d test_impl() {
return _mm_ceil_pd(_mm_set_sd(0.0));
}

[[gnu::flatten]]
__m128d test() {
return test_impl();
}

fatal error: error in backend: Cannot select: 0x497142f0: v2f64 = X86ISD::VRNDSCALE 0x49714520, TargetConstant:i32<2>, example.c:5:12 @[ example.c:10:12 ]
0x49714520: v2f64 = BUILD_VECTOR ConstantFP:f64<0.000000e+00>, ConstantFP:f64<0.000000e+00>, example.c:5:12 @[ example.c:10:12 ]
0x49714440: f64 = ConstantFP<0.000000e+00>
0x49714440: f64 = ConstantFP<0.000000e+00>
0x49714210: i32 = TargetConstant<2>
In function: test

Using `[[gnu::always_inline]]` on `test_impl` instead of using `[[gnu::flatten]]` on `test` gives a helpful error message instead (https://godbolt.org/z/d5ebWj47n):

<source>:9:5: error: always_inline function 'test_impl' requires target feature 'sse4.1', but would be inlined into function 'test' that is compiled without support for 'sse4.1'
9 | test_impl();
| ^
1 error generated.
Compiler returned: 1

I am not entirely sure if the same helpful error message should be generated for the initial example, or if the target mismatch should overrule the flatten attribute instead, such that the function is simply not inlined. FWIW, gcc seems to do the 2nd option (https://godbolt.org/z/6Taf8qhzT). But either option seems preferable to the not so helpful fatal error.
</details>