LLVM 17 breaks inlining callees with compatible target attributes

[kalcutter]

Consider the following code:

#include <immintrin.h>

__attribute__((target("avx512bw")))
static __attribute__((always_inline)) __m512i MM512_MASK_ADD_EPI8(__m512i src,
                                                                  __mmask64 k,
                                                                  __m512i a,
                                                                  __m512i b) {
    __asm__("vpaddb\t{%3, %2, %0 %{%1%}" : "+v"(src) : "Yk"(k), "v"(a), "v"(b));
    return src;
}

__attribute__((target("avx512bw")))
__m512i F(__m512i src, __mmask64 k, __m512i a, __m512i b) {
    return MM512_MASK_ADD_EPI8(src, k, a, b);
}

__attribute__((target("avx512bw,avx512dq")))
__m512i G(__m512i src, __mmask64 k, __m512i a, __m512i b) {
    return MM512_MASK_ADD_EPI8(src, k, a, b);
}

__attribute__((target("avx512bw,avx512vl")))
__m512i H(__m512i src, __mmask64 k, __m512i a, __m512i b) {
    return MM512_MASK_ADD_EPI8(src, k, a, b);
}

When compiling with previous versions of clang (up to and including version 16), MM512_MASK_ADD_EPI8 is inlined into F, G, and H (as expected) . Testing with LLVM 17 RC yields a different result: only F allows inlining and horrible code is generated for G and H. I believe this regression is caused by d6f994a.

Please revert that change or apply the appropriate fix to X86TargetTransformInfo.cpp for the LLVM 17 release.

[tru]

@kazutakahirata

[llvmbot]

@llvm/issue-subscribers-backend-x86

[marcauberer]

https://godbolt.org/z/rPWd1rhYK

[nikic]

I think we should revert this change. I've seen this cause issues on multiple instances already. always_inline overriding the target feature check was a feature, not a bug.

[kazutakahirata]

Sorry, I somehow missed this. Let me take a look.

[kazutakahirata]

It looks like X86TTIImpl::areInlineCompatible is rejecting the inlining opportunity because calls in the callee may become ABI-incompatible as a result of inlining. Eventually, we get to:

        // We don't know the target features of the callee,
        // assume it is incompatible.
        return false;

Now, the only call in the callee in this case is the inline asm, which shouldn't pose a problem in terms of the ABI compatibility. Disregarding inline asm like so around X86TargetTransformInfo.cpp:6063 fixes the problem:

  for (const Instruction &I : instructions(Callee)) {
    if (const auto *CB = dyn_cast<CallBase>(&I)) {
      if (CB->isInlineAsm())
        continue;

I'll turn this into a pull request with a test case.

[nikic]

That's a reasonable change, but this is only a specific issue. Others I've seen are calls being compatible in ways that LLVM does not understand (different target features that do not affect ABI) and the TTI hook not being implemented by all targets (IIRC PowerPC was one of the targets that doesn't, resulting in altivec inlining issues).

We should fix such cases to make general inlining more powerful, but IMHO the change to always_inline behavior is still questionable, at least without a comprehensive review of TTI hooks for accuracy.