Skip to content
This repository was archived by the owner on Apr 25, 2025. It is now read-only.

Update text syntax to use (memory $i) #25

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Contributor

This commit intends to apply #17 to the test suite by using (memory ...) instead of bare indices which can be confused with other proposals
such as SIMD which use bare indices for other purposes.

This commit intends to apply WebAssembly#17 to the test suite by using `(memory
...)` instead of bare indices which can be confused with other proposals
such as SIMD which use bare indices for other purposes.
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks! This makes sense, at least for load/store instructions. However, for memory.* instructions it's a bit odd, since in all other cases where an entity.xyz instruction is followed by an $entityidx, we don't require an extra keyword. So for consistency, I'd leave it out for those.

I would prefer holding off landing this until a corresponding change to the interpreter is implemented, since that should always stay green on the test suite.

@rossberg
Copy link
Member

rossberg commented Nov 19, 2021

Now that SIMD has landed, I created a PR for merging upstream (#26). This required adapting the parser to handle both memory indices and lane indices for lane load/stores. In doing so, I did not actually encounter any grammar ambiguities with that. In the interest of overall consistency with other memory instructions and the likes, I hence decided not to implement the syntax change suggested in this PR. Instead, #26 adds a few syntax tests to memory-multi.wast that check all combinations of immediates for lane load/stores.

(I had to do a minor grammar transformation to avoid yacc conflicts, see parser.mly#461 and parser.mly#500. However, using (memory $x) would in fact have required much more substantial transformations.)

So I'd be inclined to close this PR. Does that sound okay?

@alexcrichton
Copy link
Contributor Author

Looking at this again I agree it's not ambiguous. If you'd like to keep the syntax the way it is then that seems like what it'll be then.

@rossberg
Copy link
Member

Okay, thanks. I'm closing this PR then.

@rossberg rossberg closed this Nov 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants