Stream: git-wasmtime

Topic: wasmtime / PR #3056 aarch64: Fix incorrect encoding of la...

Wasmtime GitHub notifications bot (Jul 03 2021 at 11:52):

afonso360 opened PR #3056 from aarch64-fix-overflow-imm to main:

When encoding constants as immediates into an RSE Imm12 instruction we need to take special care to check if the value that we are trying to input does not overflow its type when viewed as a signed value. (i.e. iconst.i8 200)

We cannot both put an immediate and sign extend it, so we need to lower it into a separate reg, and emit the sign extend into the instruction.

For more details see the cg_clif bug report.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 03 2021 at 12:17):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 12:17):

bjorn3 created PR review comment:

On CI:

thread 'worker #1' panicked at 'attempt to subtract with overflow', cranelift/codegen/src/isa/aarch64/lower.rs:450:42

Wasmtime GitHub notifications bot (Jul 03 2021 at 12:48):

afonso360 updated PR #3056 from aarch64-fix-overflow-imm to main.

Wasmtime GitHub notifications bot (Jul 03 2021 at 12:49):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 12:49):

afonso360 created PR review comment:

Oops, should be fixed.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:19):

cfallin created PR review comment:

This feels somehow like the wrong place to be handling this. IMHO, iconst.i8 200 should be rejected by the CLIF validator, because the given constant value is out-of-range for the i8 type. Instead we're saying here that we should implement a wrapping behavior, but then we're only doing it in one backend (so then really we should address it in all backends instead).

I'm curious what others think, but to me this seems like a weird corner of IR semantics that is safer to define as "disallowed"...

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:19):

cfallin created PR review comment:

Can we just do this as: let is_negative = i.bits & (1 << (out_ty_bits - 1)) != 0 (i.e., check the MSB / sign bit)?

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:22):

cfallin created PR review comment:

(Nevermind this, resolving comment.)

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:38):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 18:38):

bjorn3 created PR review comment:

iconst.i8 0xff00 should result in the non-negative value 0. There is no guarantee that the immediate is either zero or sign extended. Only the n least significant bits should be considered for an n bit big type.

Wasmtime GitHub notifications bot (Jul 03 2021 at 19:07):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 19:07):

cfallin created PR review comment:

For clarity, @bjorn3, are you referring to the original or the suggested change? I think that checking the MSB only (1 << 7 in the i8 case) will correctly indicate that iconst.i8 0xff00 is non-negative when masked to the proper width; whereas the bits > ty_signed_max logic will indicate it is negative.

Wasmtime GitHub notifications bot (Jul 03 2021 at 19:09):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 19:09):

bjorn3 created PR review comment:

With your version it is ok, though I think you misplaced a paren. (- 1)) instead kf ) -1))

Wasmtime GitHub notifications bot (Jul 03 2021 at 19:09):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 03 2021 at 19:09):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 03 2021 at 20:25):

afonso360 updated PR #3056 from aarch64-fix-overflow-imm to main.

Wasmtime GitHub notifications bot (Jul 03 2021 at 20:28):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 20:28):

afonso360 created PR review comment:

Can we just do this as: let is_negative = i.bits & (1 << (out_ty_bits - 1)) != 0 (i.e., check the MSB / sign bit)?

Had to change it slightly because it was underflowing in out_ty_bits.

iconst.i8 0xff00 should result in the non-negative value 0

This one I feel like we should forbid? It feels like malformed input.

Wasmtime GitHub notifications bot (Jul 03 2021 at 20:55):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 20:55):

cfallin created PR review comment:

iconst.i8 0xff00 should result in the non-negative value 0

This one I feel like we should forbid? It feels like malformed input.

Hmm, this is actually interesting: in the absence of signedness (until use), there are two possible ranges (-128..127 or 0..255 in the i8 case). If we allow the union of these, we have a 1.5*2^N-sized range, which is a little weird, but makes some sense. And, while it shouldn't be authoritative for our case, I was curious and just checked what wat2wasm does: it accepts exactly that range in the analogous case (I tested i32.const with -2^31..2^32-1), but roundtrips the values as signed unconditionally.

So, this feels like a reasonable behavior to me: we should validate in the CLIF validator that const values are in either signed or unsigned range for the type.

This can be a followup fix, though; no need to solve it here.

With your version it is ok, though I think you misplaced a paren. (- 1)) instead of ) -1))

It's actually inside the paren correctly, I think: we want the next-lower power of two (bit 7 for an i8), not an all-ones mask. I think @afonso360's latest commit got this right fwiw.

Wasmtime GitHub notifications bot (Jul 03 2021 at 21:42):

afonso360 updated PR #3056 from aarch64-fix-overflow-imm to main.

Wasmtime GitHub notifications bot (Jul 03 2021 at 22:03):

afonso360 created PR review comment:

This can be a followup fix, though; no need to solve it here.

Let me create an issue for this

Wasmtime GitHub notifications bot (Jul 03 2021 at 22:03):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 23:05):

cfallin created PR review comment:

Thanks!

Wasmtime GitHub notifications bot (Jul 03 2021 at 23:05):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 03 2021 at 23:05):

cfallin merged PR #3056.

Last updated: Dec 23 2024 at 12:05 UTC