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

16 bit pointer fixes #74693

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

16 bit pointer fixes #74693

wants to merge 3 commits into from

Conversation

carlos4242
Copy link
Contributor

@carlos4242 carlos4242 commented Jun 25, 2024

These changes fix some crashes when trying to build programs for 16-bit targets.

Note: I'm not sure if there are any 16-bit pointer targets yet for Swift. I hope to add AVR in the near future.

The AVR architecture is extremely space constrained and will use the "embedded" compiler mode, which makes some of the fixes here not important for that platform, but they're included for completeness, in case someone wants to add other 16-bit platforms in the future where embedded mode is not used and where metadata is still important.

@phausler
Copy link
Contributor

@swift-ci please test

@carlos4242
Copy link
Contributor Author

Quick comment, I moved over a change to this PR that was going to the AVR architecture branch, because it's actually more accurately a 16 bit pointer change (see discussion on #74818 for details).

@rauhul rauhul mentioned this pull request Jul 2, 2024
9 tasks
@kubamracek
Copy link
Contributor

I think this is on the right track, but will need tests, but IIUC we can't really have any tests that target AVR until the target triple is accepted by the compiler (not even parse-only tests)? Let's then work on getting the target triple accepted first, I guess in #74818 ?

@benrimmington

This comment was marked as outdated.

@carlos4242

This comment was marked as outdated.

add a sensible default for atomics on 16 bit platform
@carlos4242
Copy link
Contributor Author

@kubamracek as I said on the call, some of these fixes may not be necessary for code compiled in embedded mode. But, theoretically, some 16-bit platforms may still want to use things like metadata and resilient classes some day. So it's really a question of whether we want to do these fixes now to help anyone in that situation? Or is it better to address this PR strictly for what's needed for AVR, and remove these fixes? It was a bit tiresome tracking down these issues and creating these fixes, but that might have been more my lack of experience than anything else.

emitClassResilientInstanceSizeAndAlignMask - I'm guessing resilient class layout isn't relevant for AVR
emitMetadataAccessByMangledName - the name suggests this isn't relevant for embedded mode AVR
TypeMetadataDemanglingCacheVariable - probably not necessary for embedded AVR?

If we omit all the above, that probably just leaves us with getExtraInhabitantIndex. Are extra inhabitants still used on embedded mode Swift, even without metadata or runtime?

@eeckstein
Copy link
Contributor

some 16-bit platforms may still want to use things like metadata and resilient classes some day.

I think the metadata runtime will never even closely fit onto a 16-bit platform.

@rauhul
Copy link
Contributor

rauhul commented Jul 8, 2024

+1 I think we should not try to support full swift for 16bit triples. Related I think we need to improve our diagnostics when building with the wrong mode, see: #75077

@rauhul rauhul requested a review from kubamracek July 8, 2024 20:13
@rauhul rauhul added the embedded Embedded Swift label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
embedded Embedded Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants