Skip to content

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

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Sep 15, 2021

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.

@beccadax
Copy link
Contributor Author

@swift-ci please test Apple Silicon

@beccadax
Copy link
Contributor Author

@swift-ci please test

@beccadax
Copy link
Contributor Author

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.

Copy link
Member

@etcwilde etcwilde left a 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() &&
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@nkcsgexi nkcsgexi left a 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>.
@beccadax
Copy link
Contributor Author

@swift-ci smoke test and merge

@beccadax
Copy link
Contributor Author

Failure is unrelated; force-merging to clear Apple Silicon bot.

@beccadax beccadax merged commit ea68a8b into swiftlang:main Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants