-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Override -triple on fallback to arm64e interface #39315
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
Conversation
@swift-ci please test Apple Silicon |
@swift-ci please test |
Once the present test runs have finished, I'll push the re-enablement of stdlib/Reflection_objc.swift, which appears to have been related to this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true that all sub-architectures are compatible, or just in the case of arm64 and arm64e?
// architecture slice. Use the original subarchitecture. | ||
llvm::Triple parsedTargetTriple(subInvocation.getTargetTriple()); | ||
if (parsedTargetTriple.getSubArch() != originalTargetTriple.getSubArch() && | ||
parsedTargetTriple.getArch() == originalTargetTriple.getArch() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style thoughts, but I usually like to throw massive comparison-chains in a std::tie
e.g.
if (pTT.getSubArch() != oTT.getSubArch() &&
std::tie(pTT.getArch(), pTT.getVendor(), pTT.getOS(), pTT.getEnvironment()) == std::tie(oTT.getArch(), oTT.getVendor(), oTT.getOS(), oTT.getEnvironment())) {
//...
}
I feel like it hides the redundant ==
and shows what is equal and what is different more clearly.
I'll leave it up to you, but just throwing it out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's going to give us a really, really really long line (and also cause a merge conflict), so I think I'll leave this as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
When we fall back to loading an arm64e module interface during an arm64 build, we want to compile it for the arm64 target so that it is fully compatible with the module that will load it, even though the flags in the file specify the arm64e target. Rewrite the sub-invocation's TargetTriple property in this specific situation. If the two targets differ by more than just the sub-architecture, we will continue to respect the -target flag in the file. Fixes <rdar://83056545>.
b9ec479
to
e16cf30
Compare
@swift-ci smoke test and merge |
Failure is unrelated; force-merging to clear Apple Silicon bot. |
When we fall back to loading an arm64e module interface during an arm64 build, we want to compile it for the arm64 target so that it is fully compatible with the module that will load it, even though the flags in the file specify the arm64e target. Rewrite the sub-invocation's
TargetTriple
property in this specific situation. If the two targets differ by more than just the sub-architecture, we will continue to respect the-target
flag in the file.Fixes rdar://83056545. Corrects implementation of #39083.