Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check for incompatible target features

Open
lhames opened this issue Oct 12, 2021 · 6 comments
Open

Check for incompatible target features #51498

lhames opened this issue Oct 12, 2021 · 6 comments
Labels
bugzilla

Comments

@lhames
Copy link
Contributor

lhames commented Oct 12, 2021

Bugzilla Link 52156
Version trunk
OS All
CC @topperc,@dwblaikie,@echristo,@efriedma-quic,@yotann

Extended Description

In #51373 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" }

@topperc
Copy link
Collaborator

topperc commented Oct 12, 2021

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.

@dwblaikie
Copy link
Collaborator

dwblaikie commented Oct 12, 2021

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.

@lhames
Copy link
Contributor Author

lhames commented Oct 12, 2021

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 #51373 ), so I don't think it's common enough to justify dedicating resources to it right now.

@topperc
Copy link
Collaborator

topperc commented Oct 12, 2021

I forgot there is TargetTransformInfo::areInlineCompatible. That's used for inlining and areFunctionArgsABICompatible is used for argument promotion.

@lhames
Copy link
Contributor Author

lhames commented Oct 12, 2021

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.

@efriedma-quic
Copy link
Collaborator

efriedma-quic commented Oct 13, 2021

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.

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla
Projects
None yet
Development

No branches or pull requests

4 participants