[WASM] Add minimal support for targeting wasm32-unknown-unknown-wasm. #20684
Conversation
- This is nothing beyond preventing the compiler from rejecting this target triple. - There doesn't yet appear to be a well-defined convention around a triple which detects WASM based on an "OS" field, so we use the "wasm" indicator in the environment component.
|
@swift-ci please smoke test |
|
This needs tests. |
|
@gottesmm it isn’t worth adding tests yet because nothing works until a few more patches land. Also, we have entire platforms (Haiku) with no discernible tests. 😛 One it starts to work, this will be implicitly covered by real tests that actually cover interesting behavior. |
|
@ddunbar I think the example of Haiku is irrelevant to this. That to me is a weak argument to not add a test. And the driver accepting a triple /is/ interesting behavior. The specific test that I would like to be added is something that shows the driver can parse the triple and not fail. Consider the following command line: With your change, this should no longer fail, no? That is the test I am asking for (one for 32 and one for 64). |
|
@gottesmm sure, I could add that, but I really don't think it adds much value for a platform and level of support that is incredibly experimental and might change. I do think having an end-to-end test once it is possible to actually compile something is useful, and at that point it will basically cover the same code. Testing before then just to me feels like testing the exact implementation. If you are super passionate about it I will add it though! 🤷♂️ |
|
I would be fine with that as long as you have a test of some sort relatively soon (maybe something that produces sil or llvm-ir). If Haiku truly doesn't have any tests (as you say), it is unfortunate and something that we should attempt to avoid for wasm. |
|
Hi @ddunbar @DougGregor, is there anything that prevents this PR from being merged? Thanks! |
| @@ -192,6 +192,8 @@ std::pair<bool, bool> LangOptions::setTarget(llvm::Triple triple) { | |||
| addPlatformConditionValue(PlatformConditionKind::OS, "PS4"); | |||
| else if (Target.isOSHaiku()) | |||
| addPlatformConditionValue(PlatformConditionKind::OS, "Haiku"); | |||
| else if (Target.isOSBinFormatWasm()) | |||
There was a problem hiding this comment.
I don't think that we should use the OSBinFormat check here. The problem is that WASM supports both ELF and WASM as object file formats, so if someone does wasm-unknown-none-elf that is going to give an error. I think that we can check if the "architecture" is wasm, and use that to indicate that we are targeting WASM.
There was a problem hiding this comment.
But I agree that the 'os' platform condition shouldn't be determined by the file format. If we're going to present wasm as an OS, I think it should be a kind of "default" we use when the triple's arch is wasm32/64 and the OS & vendor are both unknown. It's not exactly the same, but IMO that would be closer to what clang does.
|
Hi @ddunbar @DougGregor again, is there anything that prevents this PR from being merged? It's been another month. Thank you! |
|
I am against this even more than before. There has been no movement on this PR which suggests to me that any tests that were said to be added for this will not be coming in a timely way. This PR should have some sort of tests. Maybe that means we should wait until we can write an actual test for this to land... |
|
How does one add such a test? |
|
@turbolent I think a simple test would be the one in
I'm going to hold this PR open for the time being, but if there's no movement here in a while I'm going to close it out and convert it into an SR for another contributor to pick up and take over the goal line. |
|
Yeah, definitely. Now that LLVM supports |
This is nothing beyond preventing the compiler from rejecting this target
triple.
There doesn't yet appear to be a well-defined convention around a triple
which detects WASM based on an "OS" field, so we use the "wasm" indicator in
the environment component.