Stream: git-wasmtime

Topic: wasmtime / PR #3993 x64: port `load` to ISLE

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:09):

abrown opened PR #3993 from isle-load to main:

This change moves the majority of the lowerings for CLIF's load
instruction over to ISLE. To do so, it also migrates the previous
mechanism for creating an Amode (lower_to_amode) to several ISLE
rules (see to_amode).

<!--

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 (Apr 04 2022 at 21:24):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin created PR review comment:

This would be a little clearer with the specific condition in the name I think -- maybe const_shift_lt_eq_3 or somesuch?

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin created PR review comment:

I think ishl_imm is converted during legalization to ishl + iconst so we can probably remove this TODO.

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin created PR review comment:

I think we need to check whether c + offset fits in the constant immediate, right?

If so, I think we can do this with something like (it's a hack, but):

rule (to_amode flags (iadd (iconst (low32_will_sign_extend_to_64 c)) base) (u32_add <c offset)))

in other words, make u32_add an extractor as well, and take an in-arg (use arg polarity for this) with the half of the sum we already know (c). For the arg-flipped version below we need to use the u32_add extractor inside the low32_will_sign_extend... instead.

(open to ideas how we might clean this up in ISLE, by the way -- aside from fitzgen's ideas around higher-order extractors, I kind of like the notion of separate constraint clauses as well, which would allow us to write something like (given (u32_add_fits offset c)) after the LHS...)

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin created PR review comment:

s/c >> amount/c << amount/, I think?

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:27):

cfallin created PR review comment:

From the implementation below it looks like it's actually checking whether the value will fit in a u32, so basically, positive values <= 4GiB? Is that right? Do we want to check <= 2GiB or -2GiB <= x <= 2GiB instead?

(pre-existing confusion I know but it's worthwhile to fix this while we're here!)

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:33):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:44):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:44):

abrown created PR review comment:

Done; it would be nice to have arithmetic expressions in ISLE somehow, a la (shift <= 3).

Wasmtime GitHub notifications bot (Apr 04 2022 at 21:46):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 04 2022 at 23:53):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 04 2022 at 23:54):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 04 2022 at 23:54):

abrown created PR review comment:

Good catch; that logic was all wrong. Take a look at the new helpers and their logic.

Wasmtime GitHub notifications bot (Apr 04 2022 at 23:56):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 06 2022 at 23:07):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 06 2022 at 23:14):

abrown has marked PR #3993 as ready for review.

Wasmtime GitHub notifications bot (Apr 06 2022 at 23:15):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 06 2022 at 23:15):

abrown created PR review comment:

I don't like having two of these weird helpers doing almost the same thing. It would be great if some of this logic could just exist in ISLE, but in the meantime... suggestions appreciated.

Wasmtime GitHub notifications bot (Apr 07 2022 at 03:44):

abrown requested cfallin for a review on PR #3993.

Wasmtime GitHub notifications bot (Apr 07 2022 at 18:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2022 at 18:45):

cfallin created PR review comment:

IIRC (and I might not, this stuff is subtle!) I think this comment should be adjusted slightly: negative values are fine if they sign-extend correctly, no? (The logic looks correct below, concern is just with the comment.)

Wasmtime GitHub notifications bot (Apr 07 2022 at 18:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2022 at 18:45):

cfallin created PR review comment:

bits() returns an i64, so won't the .try_into().unwrap() panic if given a negative value in the Imm64? I think since we're working with raw bits here then determining possibility of representation below, we can cast the raw bits with as u64.

Wasmtime GitHub notifications bot (Apr 07 2022 at 18:45):

cfallin created PR review comment:

I think this will use a movzx for 32-bit loads as well? Strictly speaking this should be fine, and it looks like it matches how loads happen today actually, but it might be a bit more idiomatic, and I think one byte shorter in encoding, to use a 32-bit mov instead.

Wasmtime GitHub notifications bot (Apr 07 2022 at 18:45):

cfallin created PR review comment:

Longer-term I want to extend ISLE semantics a bit to make this work without the in-args, as we talked about (for others watching, I'll write more about some ideas soon!). For now I wonder if we could collapse these two extractors into one by passing a default type like $I64 to the Type for the non-extend case?

Wasmtime GitHub notifications bot (Apr 07 2022 at 20:01):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2022 at 20:01):

abrown created PR review comment:

Ok, changed.

Wasmtime GitHub notifications bot (Apr 07 2022 at 22:59):

abrown updated PR #3993 from isle-load to main.

Wasmtime GitHub notifications bot (Apr 07 2022 at 23:56):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 07 2022 at 23:56):

abrown created PR review comment:

I'll add a TODO issue for this; I have been fuzzing with no issues but, being a bit paranoid, want to let oss-fuzz check this for a while before changing the logic: https://github.com/bytecodealliance/wasmtime/issues/4006.

Wasmtime GitHub notifications bot (Apr 08 2022 at 01:31):

abrown merged PR #3993.

Last updated: Dec 23 2024 at 12:05 UTC