[X86] Allow inlining callees missing VLX feature

[kalcutter]

Summary

This patch attempts to fix a regression caused by https://github.com/llvm/llvm-project/commit/d6f994acb3d545b80161e24ab742c9c69d4bbf33. In particular, always_inline should work on callees without VLX from VLX functions. I found this testing clang-17-rc2. If accepted, please also apply this to the LLVM 17.0.0 branch.

[nikic]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6082

I assume that this is the actually failing check? In that case, should the adjustment be in that function?

[kalcutter]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6082

I am not sure. That function is checking that the used types are ABI compatible, but what about available target instructions? Is the idea to first only check the types, then later during CodeGen the target does a more exhaustive check?

Fixing areTypesABICompatible() would be more general I guess. Do you have an idea how much work is involved properly fixing that function? I am not familiar with this code base at all. Do you have anything against applying this patch as an incremental improvement?

[kalcutter]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6082

I guess the feature subset check covers all cases of available target instructions. So just making sure all the types are compatible, like you suggested, would be the proper fix.

[kalcutter]

There is also another related issue: When areInlineCompatible() returns false here, a callee with attribute always_inline is silently not inlined. It seems like clang should emit and error or at least a warning in this case. I only noticed this because my code was running 10x slower compiling with clang 17.

[pengfei]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6082

The function is designed to check if both caller and callee allow ZMM codegen or not when passing a 512-bit vector type. The logic to check ZMM codegen is:

1. has AVX512F but not AVX512VL
2. has AVX512F and prefer-vector-width >= 512
3. has AVX512F and min-legal-vector-width > 256

So has AVX512VL or not does affect the ABI when prefer-vector-width and min-legal-vector-width are set 256 so less, though it shouldn't happen since we have guarantee they are set properly in the FE.

[kalcutter]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6082

I am a bit confused. A function with both AVX512F and AVX512VL features obviously allows ZMM codegen in the function body. Why should enabling AVX512VL change how 512-bit vector types are passed? Even if that were the case for an opaque function, why should that disabling inlining? Also I don't see how (1)-(3) should ever affect`always_inline`.

[pengfei]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6082

We have an option -mprefer-vector-width=128/256, which prefers to 128/256 vector instructions under AVX512VL. Under such situation, 512-bit vector may be split into 256-bit when passing. No sure if it is the reason we disable inlining.

[kalcutter]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6058

Thinking about this more. Why shouldn't this function just end at this line? ISTM that only features should control if inlining is _possible_ or not. I don't see how checking for ABI compatibility between types is helpful. Once the callee gets inlined, it can take on the ABI of the caller anyway.

[pengfei]

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

6058

It's good for caller has FeatureVLX but callee doesn't, but maybe we need to check ABI compatibility for the both direction. The reason we need to check type is only vector type affected by feature difference.

llvm/lib/Target/X86/X86TargetTransformInfo.cpp

const FeatureBitset &CalleeBits =
       TM.getSubtargetImpl(*Callee)->getFeatureBits();
 
   // Check whether features are the same (apart from the ignore list).
   FeatureBitset RealCallerBits = CallerBits & ~InlineFeatureIgnoreList;
   FeatureBitset RealCalleeBits = CalleeBits & ~InlineFeatureIgnoreList;
   if (RealCallerBits == RealCalleeBits)
     return true;
 
+  // If the callee is only missing VLX, they are compatible.
+  if (RealCallerBits == (RealCalleeBits | FeatureBitset{X86::FeatureVLX}))
+    return true;
+
   // If the features are a subset, we need to additionally check for calls
   // that may become ABI-incompatible as a result of inlining.
   if ((RealCallerBits & RealCalleeBits) != RealCalleeBits)
     return false;
 
   for (const Instruction &I : instructions(Callee)) {
     if (const auto *CB = dyn_cast<CallBase>(&I)) {
       SmallVector<Type *, 8> Types;
       for (Value *Arg : CB->args())
         Types.push_back(Arg->getType());
       if (!CB->getType()->isVoidTy())
         Types.push_back(CB->getType());
 
       // Simple types are always ABI compatible.
       auto IsSimpleTy = [](Type *Ty) {
         return !Ty->isVectorTy() && !Ty->isAggregateType();
       };
       if (all_of(Types, IsSimpleTy))
         continue;
 
       if (Function *NestedCallee = CB->getCalledFunction()) {
         // Assume that intrinsics are always ABI compatible.
         if (NestedCallee->isIntrinsic())
           continue;
 
         // Do a precise compatibility check.
         if (!areTypesABICompatible(Caller, NestedCallee, Types))
           return false;
       } else {
         // We don't know the target features of the callee,
         // assume it is incompatible.
         return false;
       }
     }
   }

llvm/test/Transforms/Inline/X86/call-abi-compatibility.ll

 }
 
 define internal void @caller_not_avx4() {
   call i64 @caller_unknown_simple(i64 0)
   ret void
 }
 
 declare i64 @caller_unknown_simple(i64)
+
+; This call should get inlined, because the callee is only missing VLX.
+define void @caller_vlx() "target-features"="+avx512f,+avx512vl" {
+; CHECK-LABEL: define {{[^@]+}}@caller_vlx
+; CHECK-SAME: () #[[ATTR2:[0-9]+]] {
+; CHECK-NEXT:    call void @callee_not_vlx(<8 x i64> <i64 0, i64 1, i64 2, i64 3, i64 4, i64 5, i64 6, i64 7>)
+; CHECK-NEXT:    ret void
+;
+  call void @caller_not_vlx(<8 x i64> <i64 0, i64 1, i64 2, i64 3, i64 4, i64 5, i64 6, i64 7>)
+  ret void
+}
+
+define internal void @caller_not_vlx(<8 x i64> %arg) "target-features"="+avx512f" {
+  call void @callee_not_vlx(<8 x i64> %arg)
+  ret void
+}
+
+declare void @callee_not_vlx(<8 x i64>)