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

Refactor relaxed ops spec #1799

Merged
merged 2 commits into from
Sep 7, 2024
Merged

Refactor relaxed ops spec #1799

merged 2 commits into from
Sep 7, 2024

Conversation

rossberg
Copy link
Member

@rossberg rossberg commented Sep 4, 2024

I'm in the process of merging Relaxed SIMD into the wasm-3.0 branch. As part of that, this PR applies tweaks to the specification of relaxed operators:

@ngzhian, could you please have a look?

@rossberg rossberg requested a review from ngzhian September 4, 2024 15:19
@rossberg
Copy link
Member Author

rossberg commented Sep 4, 2024

FWIW, I noticed that there is no correlation between the implementation choices of the semantics for fmin and fmax. Should there be? With the explicit global parameters, that would now be easy to express.

@ngzhian
Copy link
Member

ngzhian commented Sep 5, 2024

took a quick look, LG, thanks for the fixes! I would get @dtig to look more (i don't work on this anymore)

@ngzhian
Copy link
Member

ngzhian commented Sep 5, 2024

FWIW, I noticed that there is no correlation between the implementation choices of the semantics for fmin and fmax. Should there be? With the explicit global parameters, that would now be easy to express.

based on WebAssembly/relaxed-simd#33, yes fmin and fmax should be correlated.

Copy link
Member

@dtig dtig left a comment

Choose a reason for hiding this comment

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

Paging this back in, will take a closer look tomorrow - the one thing I'm mildly uncomfortable (non-blocking) about is the detail around renaming host-dependent behavior to implementation-dependent, mostly because that doesn't capture the dependency on the host very well. I'm wondering what you think of including a non-normative implementation note in numerics.rst or somewhere else that's appropriate to capture that the fixed set of results are fully dependent on the host environment, and the hardware that the program is run on.

@rossberg
Copy link
Member Author

rossberg commented Sep 6, 2024

@dtig, good point about calling that out. I added an implementation note. Let me know what you think.

FWIW, there are multiple reasons why I rephrased from host-dependent to implementation-dependent. For one, we have been using "host" quite ambiguously, referring to different things, including either platform or embedder. In the spec, I so far tried to only use it the sense of embedder, which is not what it was referring to here.

Then, from a semantic perspective, it is not generally meaningful to differentiate where a semantic choice is coming from. It may not be the host environment, e.g., consider an emulator. Whether an implementation uses fast hardware behaviour is a performance / quality-of-implementation property.

Moreover, there are valid reasons for implementations to decide against exposing host behaviour — the deterministic profile is the obvious example. We deliberately allow that, hence "host-dependent" arguably is too specific to describe the space of choices.

Does that make sense?

Copy link
Member

@dtig dtig left a comment

Choose a reason for hiding this comment

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

The rephrasing to implementation-dependent makes sense, thanks for articulating the reasons for the change. I think the note clarifying the expectations captures for implementations here bridges the gap between what we were looking at previously as host-specific. Thanks for adding the note.

@rossberg rossberg merged commit bc87071 into wasm-3.0+relaxed Sep 7, 2024
@rossberg rossberg deleted the wasm-3.0+relaxed.ops branch September 7, 2024 07:35
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