Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Add Python pseudocode to memory operations #107

Merged
merged 7 commits into from
Nov 18, 2019

Conversation

penzn
Copy link
Contributor

@penzn penzn commented Sep 13, 2019

Introducing pseudocode for memory operations.

@penzn
Copy link
Contributor Author

penzn commented Sep 13, 2019

@tlively @ngzhian what do you think about this style?

Change semantics for memory operations to use `from_bytes` to get the
lane or vector values from byte array (assume that it would work on `S`
type). Change `memory.bytes` -> `memory`, use `bytes(a)` as a way to
convert a value to a bytestring.
@penzn penzn requested review from ngzhian and tlively September 13, 2019 23:32
@ngzhian ngzhian requested a review from dtig September 16, 2019 17:16
Copy link
Member

@ngzhian ngzhian left a comment

Choose a reason for hiding this comment

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

LGTM, pulling in Deepti for comments (if any), thank you Petr for these changes!

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.

Sorry this is very delayed, I missed that I was added as a reviewer on this.

The functions below are all memory operations, and they should use the memarg correctly, i.e. currently it looks like it's only reading from the start of the memory, but memarg contains an address offset and the expected alignment that is not accounted for here. Offset should be used to calculate effective address, and even though unaligned loads/stores are supported, I think the alignment hint here should be specified.

@penzn
Copy link
Contributor Author

penzn commented Nov 13, 2019

Sorry, I lost track of this. I was under impression that alignment hint should not affect the address, that's why only offset is used in this PR. Alignment has seemed a bit of a murky area to me, any clarity would be greatly appreciated :)

From docs on webassembly.org:

If the effective address of a memory access is a multiple of the alignment attribute value of the memory access, the memory access is considered aligned, otherwise it is considered misaligned. Aligned and misaligned accesses have the same behavior.

Also, similar language in current spec on github:

The alignment \memarg.\ALIGN in load and store instructions does not affect the semantics. It is an indication that the offset \X{ea} at which the memory is accessed is intended to satisfy the property \X{ea} \mod 2^{\memarg.\ALIGN} = 0. A WebAssembly implementation can use this hint to optimize for the intended use.

@tlively
Copy link
Member

tlively commented Nov 13, 2019

Right, alignment hints are part of the memarg but do not change semantics in any way. I'm not sure it's necessary to explicitly mention them since memargs are already specified, but perhaps linking to the relevant parts of the spec would be helpful (https://webassembly.github.io/spec/core/syntax/instructions.html#syntax-memarg, https://webassembly.github.io/spec/core/binary/instructions.html#binary-memarg). It would also be good to mention somewhere that the natural alignment for v128 is 16 bytes.

To keep the naming consistent, the pseudocode should use memarg.offset rather than memarg.start. I believe that's what @dtig was getting at as well.

@penzn
Copy link
Contributor Author

penzn commented Nov 13, 2019

Good point on offset, changed that.

@penzn penzn requested a review from dtig November 13, 2019 18:00
@dtig
Copy link
Member

dtig commented Nov 18, 2019

Sorry I wasn't clear in my earlier comment, I did mean that memarg.offset should be used instead of memory start as it's confusing to use terminology not a part of memarg, looks like that's fixed now so that part of the PR lgtm. By accounting for alignment as well, I meant that if the specified alignment is larger than the natural alignment of s128 type, that will be a validation failure, and pseudo code should probably account for that? I can see how that's probably out of scope for this PR as the rest of the text in SIMD.md doesn't talk about validation failures right now so approving. But this is more noticeable in pseudocode as that is more explicit about the operation.

@arunetm arunetm merged commit 43de658 into WebAssembly:master Nov 18, 2019
@binji
Copy link
Member

binji commented Jan 14, 2020

The semantics here are not quite right, memarg is an immediate for a load/store operation, and has an immediate offset. The instruction itself also has an i32 operand which must be included in the address calculation.

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.

6 participants