Skip to content

Update AstSemantics.md #366

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

Merged
merged 4 commits into from
Sep 23, 2015
Merged

Update AstSemantics.md #366

merged 4 commits into from
Sep 23, 2015

Conversation

titzer
Copy link

@titzer titzer commented Sep 23, 2015

It looks like loads and stores of i64 with sizes < 64bits got dropped on the floor. This PR adds them back and revises the text to no longer refer to memory types.


Linear memory access is accomplished with explicit `load` and `store` operations that
accept as input an `index` operand that is interpreted as a unsigned byte offset into the
linear memory.
Copy link
Member

Choose a reason for hiding this comment

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

They accept as input an index and an offset which are described in the Addressing section immediately following this section.

@sunfishcode
Copy link
Member

lgtm, other than a comment above.

@titzer
Copy link
Author

titzer commented Sep 23, 2015

I just removed the references to indexing, since they are redundant with
the text in the next section.

On Wed, Sep 23, 2015 at 3:19 PM, Dan Gohman [email protected]
wrote:

lgtm, other than a comment above.


Reply to this email directly or view it on GitHub
#366 (comment).

titzer pushed a commit that referenced this pull request Sep 23, 2015
Add back extending load/stores for 64-bit integers.
@titzer titzer merged commit 66bea19 into master Sep 23, 2015
@lukewagner
Copy link
Member

This PR undoes the explicit change in #326/#341/spec/#55 that @rossberg-chromium proposed and everyone (including you) lgtm'd which was to avoid including every possible coercion-on-load except those that were necessary to avoid introducing unwanted intermediate types (int8/int16). I don't have a strong opinion either way, but the change was intentional and we'll need to revise spec/ (again) with this design change.

@titzer
Copy link
Author

titzer commented Sep 24, 2015

It was an oversight on my part to not notice that the coercions for i64
were removed in rossberg's proposal.

I think it's important to keep them since otherwise load/stores working
with i64 and smaller sizes would require another explicit coercion.

On Thu, Sep 24, 2015 at 5:05 PM, Luke Wagner [email protected]
wrote:

This PR undoes the explicit change in #326
#326
#341
WebAssembly/spec#55 that @rossberg-chromium
https://github.com/rossberg-chromium proposed and everyone (including
you
#326 (comment))
lgtm'd which was to avoid including every possible coercion-on-load except
those that were necessary to avoid introducing unwanted intermediate types
(int8/int16). I don't have a strong opinion either way, but the change was
intentional and we'll need to revise spec/ (again) with this design change.


Reply to this email directly or view it on GitHub
#366 (comment).

@titzer titzer deleted the load_store_64 branch September 24, 2015 15:23
@lukewagner
Copy link
Member

Ok; this is just the usual pattern-match vs. EITBI argument. I generally lean to the latter, so I guess I'm actually happy to see this PR; I mostly just wanted to point out that the change wasn't accidental (at least for some). I can update spec/ to match this.

@lukewagner
Copy link
Member

Btw: if you search AstSemantics.md there is only one more dangling reference to memory type (in Addressing) and then we could remove "Memory type".

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