In https://llvm.org/PR52031 it was found that the module below passes the verifier, despite the two functions main and new_function having incompatible target-features. If possible, we should have the verifier query the target to detect incompatible features like this so that they can be quickly reported to users. module.ll: define i32 @main() #0 { br label %1 1: ; preds = %1, %0 %2 = phi i64 [ 0, %0 ], [ %6, %1 ] %3 = phi <4 x i64> [ <i64 -16, i64 -16, i64 -16, i64 -16>, %0 ], [ %5, %1 ] %4 = trunc <4 x i64> %3 to <4 x i32> %5 = call <4 x i64> @new_function(<4 x i64> %3) %6 = add i64 %2, 1 %7 = icmp eq i64 %6, 2 br i1 %7, label %8, label %1 8: ; preds = %1 %9 = extractelement <4 x i32> %4, i32 3 ret i32 %9 } define <4 x i64> @new_function(<4 x i64> %0) unnamed_addr { %2 = add <4 x i64> %0, <i64 16, i64 16, i64 16, i64 16> ret <4 x i64> %2 } attributes #0 = { "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" }
The feature mismatch only matters when it changes the ABI of the passed arguments. attribute(target("features")) allows functions to be compiled with different features and you could easily imagine such a function calling a library function that doesn't have the same features enabled. We have areFunctionArgsABICompatible in TTI, but it is very conservative about when it returns true. Returning false is used to block optimizations like inlining and argument promotion. I don't think it could be used in its current form for a verifier check.
(In reply to Craig Topper from comment #1) > The feature mismatch only matters when it changes the ABI of the passed > arguments. attribute(target("features")) allows functions to be compiled > with different features and you could easily imagine such a function calling > a library function that doesn't have the same features enabled. > > We have areFunctionArgsABICompatible in TTI, but it is very conservative > about when it returns true. Returning false is used to block optimizations > like inlining and argument promotion. I don't think it could be used in its > current form for a verifier check. Ah, thanks for the pointers, Craig - yeah, I was worried/thinking maybe there'd be a distinction between ABI incompatible and inline-incompatible. Good to know more about that.
Yeah -- that's good to know. So a verifier error isn't possible at the moment, and a warning based on the current infrastructure would be too noisy to be useful. How hard would it be to improve the accuracy of areFunctionArgsABICompatible? If it wouldn't be too difficult then it might make sense to leave this open in case anyone wants to look at it. Otherwise I'm inclined to close this bug: I've only seen this failure mode twice before (once in PR52031), so I don't think it's common enough to justify dedicating resources to it right now.
I forgot there is TargetTransformInfo::areInlineCompatible. That's used for inlining and areFunctionArgsABICompatible is used for argument promotion.
I guess we only care about areFunctionArgsABICompatible here (or some similar check on callsite-callee ABI compatibility)? As long as areInlineCompatible blocks inlining it doesn't really tell us anything about correctness, but knowing that a call-site and callee don't have compatible ABIs for the arguments are return value is significant.
Ultimately, I think this is harder than it needs to be because there's a missing IR feature. Suppose you have a function with the signature "void(<4 x i64>)", which is defined work with AVX disabled (so the backend splits it). Suppose you want to implement this function, and you want the implementation to use AVX. This currently doesn't work: the only ways to make this work are to either define an extra thunk to translate the argument, or intentionally define it with the "wrong" signature. Suppose that, instead of using target-features to figure out the calling convention of a function, we have a separate attribute "cc-target-features" that determines how a function like "void(<4 x i64>)" is called. If the caller and the callee specify that they want the non-avx convention, then we get that convention, even if the callee is AVX-enabled. This allows defining the implementation the way we want, and makes everything explicit. If we have this feature, we can then add a simple IR lint: if the caller and callee have mismatched cc-target-features, your IR generation is probably wrong. (It can't be a verifier error because of the usual "undefined behavior must compile" issue.) This also allows us to simplify transforms like argument promotion: as long as the "cc-target-features" match, we can promote arguments without worrying about their type. -------- Without this feature, we're stuck trying to classify calls based on the target's implementation of call lowering: basically, to figure out if a call is "safe", we need to lower a call twice, once with the caller's and once with the callee's target features, and see if we end up with equivalent code. Realistically, we wouldn't want to actually generate code, so we'd need invent some new form of lowering based on the calling convention TableGen files. This is possible, I guess, but a lot more complicated than the current approximation used by areFunctionArgsABICompatible.