Skip to content

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Aug 26, 2024

This requires an update the version of node canary that use for testing.

@sbc100 sbc100 force-pushed the use_table64 branch 6 times, most recently from 1400cf6 to 7375406 Compare August 26, 2024 23:38
@sbc100 sbc100 requested review from dschuff and kripken August 26, 2024 23:39
executor: bionic
environment:
EMTEST_SKIP_NODE_CANARY: "1"
EMTEST_SKIP_WASM64: "1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to have both this and the node canary update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This builder does not have node canary installed. So it can no longer run wasm64 tests after this change. We could install node canary, but general we only configure one version of node per bot.

'node': 150000,
},
Feature.MEMORY64: {
'chrome': 128,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it premature to add this? we don't know which versions will ship this unflagged.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone targeting memory64 is already seeing an experimental warning, so that we are really targetting here is some arbitrary interim version of memory64. Once we remove the experimental warning we will pin the final values here I imagine?


if settings.MEMORY64:
passes += ['--table64-lowering']
passes += ['--memory64-lowering', '--table64-lowering']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should --table64-lowering go away? I guess we can just roll the passes together into memory64-lowering now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, eventually, but I guess that can/should be a followup.

@sbc100 sbc100 force-pushed the use_table64 branch 3 times, most recently from a2a73ea to 7ac7fc9 Compare August 27, 2024 01:41
@kripken
Copy link
Member

kripken commented Aug 27, 2024

Do we only see table64 lowering as useful for testing? Not for users that want to only target wasm64 but are waiting for full browser adoption?

@sbc100
Copy link
Collaborator Author

sbc100 commented Aug 27, 2024

Do we only see table64 lowering as useful for testing? Not for users that want to only target wasm64 but are waiting for full browser adoption?

Are there any such targets? Now that v8 and spidermonkey both adopted table64 I don't see it as being very useful. Am I missing something?

I guess if you were not able to update to the latest browser you might be effected.. but in that case you can't really expect to target experimental features can you?

@kripken
Copy link
Member

kripken commented Aug 27, 2024

Are there any such targets? Now that v8 and spidermonkey both adopted table64 I don't see it as being very useful. Am I missing something?

WebKit/Safari still lacks support, I think? So for people that wanted to use this in production it could be a problem. But I honestly don't know if there are such people (that are ok with a 32-bit lowering of 64-bit code).

@dschuff
Copy link
Member

dschuff commented Aug 27, 2024

Don't confuse table64 lowering with full memory64 lowering (as I did when I discussed this with Sam earlier 😅) IMO keeping full memory64 lowering around for a while longer is useful and I think that is what you're saying. But table64 lowering is just for the interim stage when Chrome and Firefox supported the rest of memory64 but not table64. Presumably Safari and others won't have such an interim state, they will just implement the whole proposal at once.

@kripken
Copy link
Member

kripken commented Aug 27, 2024

Thanks, I was confused about exactly that. lgtm

@sbc100 sbc100 force-pushed the use_table64 branch 2 times, most recently from 84d9a0c to ee8e9f0 Compare August 29, 2024 16:52
This requires an update the version of node canary that use for testing.
@sbc100 sbc100 merged commit 7d78ad8 into emscripten-core:main Sep 4, 2024
2 checks passed
@sbc100 sbc100 deleted the use_table64 branch September 4, 2024 15:55
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