Bikeshedding: consistent naming conventions regarding signedness

[rossberg]

While working on making the opcodes in the ml-proto line up with the AST semantics, I noticed that it currently uses several inconsistent conventions for expressing sign awareness:

1. Conversion operators use _signed and _unsigned suffixes.
2. Load operators use _sx and _zx suffixes.
3. Arithmetic operators use s and u prefixes.

The ones under (2) and (3) have the exact same semantic meaning as several occurrences in (1), so there is no apparent reason for them to diverge.

Can naming stick to one convention? Say, _s and _u suffixes, for brevity?

[titzer]

On Fri, Aug 28, 2015 at 5:00 PM, rossberg-chromium <[email protected]

wrote:

While working on making the opcodes in the ml-proto line up with the AST
semantics, I noticed that it currently uses several inconsistent
conventions for expressing sign awareness:

1. Conversion operators use _signed and _unsigned suffixes.
2. Load operators use _sx and _zx suffixes.
3. Arithmetic operators use s and u prefixes.

The ones under (2) and (3) have the exact same semantic meaning as several
occurrences in (1), so there is no apparent reason for them to diverge.

Can naming stick to one convention? Say, _s and _u suffixes, for brevity?

+1, although I like _sx and _zx since they are extensions of a value to a
larger bitwidth, where _s and _u are just interpretation of a given width
of bits.

—
Reply to this email directly or view it on GitHub
#317.

[lukewagner]

@titzer That interpretation could also apply to sign extension: you load a memory_type temporary value, the _s/_u tells you how to interpret that fixed width of bits, and then sign extension is implied by how to preserve the interpreted signed/unsigned value. So +1 for uniform _s/_u.

@rossberg-chromium I was looking at this earlier today (about to write the patch but wanting to wait to not conflict with your general renaming/refactoring) and I additionally had wanted to make these changes to bring spec closer to design:

• rename Types.value_type to Types.local_type.
• add a local_type to memop because I think it is necessary to distinguish, e.g., int32.load_zx[int8] from int64.load_zx[int8]
• move Memory.mem_type into Types.ml and name it memory_type and remove the signedness. Instead, split memop into loadop/storeop where loadop additionally has an extension field that's one of Sign/Zero/None
• don't try to rule out invalid combinations (e.g. sign-extension of float or same-sized int) in the SExpr parser but, instead, since the invalid combinations exist in the AST, check for invalid combinations in check.ml. This would let us write tests with assertinvalid.

[titzer]

On Fri, Aug 28, 2015 at 6:18 PM, Luke Wagner [email protected]
wrote:

@titzer https://github.com/titzer That interpretation could also apply
to load/store: you load a memory_type temporary value, the _s/_u tells
you how to interpret that fixed width of bits, and then sign extension is
implied by how to preserve the interpreted signed/unsigned value. So +1 for
uniform _s/_u.

Fair point, sgtm.

@rossberg-chromium https://github.com/rossberg-chromium I was looking
at this earlier today (about to write the patch but wanting to wait to not
conflict with your general renaming/refactoring) and I additionally had
wanted to make these changes to bring spec closer to design:

• rename Types.value_type to Types.local_type.
• add a local_type to memop because I think it is necessary to
  distinguish, e.g., int32.load_zx[int8] from int64.load_zx[int8]
• move Memory.mem_type into Types.ml and name it memory_type and
  remove the signedness. Instead, split memop into loadop/storeop where
  loadop additionally has an extension field that's one of Sign/Zero/None
• don't try to rule out invalid combinations (e.g. sign-extension of
  float or same-sized int) in the SExpr parser but, instead, since the
  invalid combinations exist in the AST, check for invalid combinations in
  check.ml. This would let us write tests with assertinvalid.

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

[jfbastien]

sgtm

[rossberg]

On 28 August 2015 at 18:18, Luke Wagner [email protected] wrote:

@rossberg-chromium https://github.com/rossberg-chromium I was looking
at this earlier today (about to write the patch but wanting to wait to not
conflict with your general renaming/refactoring) and I additionally had
wanted to make these changes to bring spec closer to design:

• rename Types.value_type to Types.local_type.

I actually was about to suggest the opposite :) : rename LocalType to
ValueType in the AstSemantics. I think "local" is both imprecise and
inaccurate, e.g. because it is used for globals as well. Value type OTOH
exactly matches their role as the types of semantic values (as opposed to
e.g. memory types). WDYT?

• add a local_type to memop because I think it is necessary to
  distinguish, e.g., int32.load_zx[int8] from int64.load_zx[int8]

Already done in the latest version of the PR.

• move Memory.mem_type into Types.ml and name it memory_type and
  remove the signedness. Instead, split memop into loadop/storeop where
  loadop additionally has an extension field that's one of Sign/Zero/None

Sounds reasonable. But probably want to do that in a separate patch.

• don't try to rule out invalid combinations (e.g. sign-extension of
  float or same-sized int) in the SExpr parser but, instead, since the
  invalid combinations exist in the AST, check for invalid combinations in
  check.ml. This would let us write tests with assertinvalid.

Sounds good, too. The current PR already implements these checks in the
validator, although they cannot actually be triggered, because the lexer
never produces invalid combinations. That can easily be changed, though. I
suppose the question is whether you want to regard these as a syntactic or
semantic restriction. In particular, will the binary format even be able to
express the invalid combinations?

[rossberg]

On 29 August 2015 at 03:30, Andreas Rossberg [email protected] wrote:

On 28 August 2015 at 18:18, Luke Wagner [email protected] wrote:

• add a local_type to memop because I think it is necessary to
  distinguish, e.g., int32.load_zx[int8] from int64.load_zx[int8]

Already done in the latest version of the PR.

• move Memory.mem_type into Types.ml and name it memory_type and
  remove the signedness. Instead, split memop into loadop/storeop where
  loadop additionally has an extension field that's one of Sign/Zero/
  None

  Sounds reasonable. But probably want to do that in a separate patch.

I had another iteration on these ideas. Observation: if you add a regular
type to memop and also take out signedness from mem_type, then there isn't
much purpose left for mem_type other than specifying the access size. So
why not get rid of mem_type entirely? Instead just have a family of
operators

.load8_s|u
.load16_s|u
.load32(_s|u)?
.load64(_s|u)?
.store8
.store16
.store32
.store64

[sunfishcode]

Re: signedness, we do make a semantic distinction between signed/unsigned and sign-agnostic. The extending loads could plausibly go either way, but I agree that signed/unsigned is a little nicer. +1 for _s/_u there.

[titzer]

On Sat, Aug 29, 2015 at 5:26 PM, rossberg-chromium <[email protected]

wrote:

On 29 August 2015 at 03:30, Andreas Rossberg [email protected] wrote:

On 28 August 2015 at 18:18, Luke Wagner [email protected]
wrote:

• add a local_type to memop because I think it is necessary to
  distinguish, e.g., int32.load_zx[int8] from int64.load_zx[int8]

Already done in the latest version of the PR.

• move Memory.mem_type into Types.ml and name it memory_type and
  remove the signedness. Instead, split memop into loadop/storeop where
  loadop additionally has an extension field that's one of Sign/Zero/
  None

Sounds reasonable. But probably want to do that in a separate patch.

I had another iteration on these ideas. Observation: if you add a regular
type to memop and also take out signedness from mem_type, then there isn't
much purpose left for mem_type other than specifying the access size. So
why not get rid of mem_type entirely? Instead just have a family of
operators

.load8_s|u
.load16_s|u
.load32(_s|u)?
.load64(_s|u)?
.store8
.store16
.store32
.store64

This is basically what v8-native does. There's the local type , a
signedness bit, and an access size. The access size and signedness are only
relevant for integer accesses (i.e. no extensions for float32/float64).

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

[rossberg]

I created PR WebAssembly/spec#39 that implements this.

[sunfishcode]

The concept.of a memory type anticipates a potential future need for a load that extends from float16, though it's not necessarily essential to have consistency there.

[rossberg]

@dan, not sure I understand. Why would e.g. a `float32.load16` not cover that case within the above scheme?

[sunfishcode]

Ah, so each type implies a kind of widening conversion. That makes sense.

[rossberg]

Main suggestion resolved and implemented, closing.