Stream: git-wasmtime

Topic: wasmtime / issue #3807 Implicit type conversions in ISLE

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:43):

avanhatt commented on issue #3807:

FWIW, I tried merging this with our verification branch to try it on the iadd example (full test case here), and I'm seeing this newly added error from isle::sema in using a constructor. Am I messing up the syntax here? Also, should I be seeing the name of the term that's unknown?

called `Result::unwrap()` on an `Err` value: TypeError { msg: "Unknown term for converter", src: Source { name: "fuzz-input.isle", source: "<redacted>", span: SourceSpan { offset: SourceOffset(362), length: SourceOffset(1) } }, cranelift/isle/veri/veri_engine/src/main.rs:26:58

Example snippet (full snippet linked above):

        (type Inst (primitive Inst))
        (type Type (primitive Type))
        (type Value (primitive Value))

        (type Reg (primitive Reg))
        (type ValueRegs (primitive ValueRegs))

        ;; NEW: IMPLICIT CONVERTERS
        (convert Reg ValueRegs value_reg)
        (convert Value Reg put_in_reg)

        ;; ...
        (rule (lower (has_type (fits_in_64 ty) (iadd x y)))
            (add ty x y))

Wasmtime GitHub notifications bot (Feb 23 2022 at 19:59):

cfallin commented on issue #3807:

@avanhatt that's odd; that's a fairly basic error indicating that the lookup of the term name itself (value_reg or put_in_reg in your snippet) failed. The syntax looks right, and this feature is definitely working correctly otherwise in tests and in my exploratory use of it, so I might check if the merge introduced some issue?

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:08):

avanhatt commented on issue #3807:

@cfallin ah, it was a silly mistake, I misinterpreted the convert statement as _replacing_ the decl/extern ... statements for the term rather than being in addition to them. Looks like it works now adding those back!

I might add to the section of the docs that the converter must already be defined? This part is what I misunderstood:

Now we can define a conversion such as

 ```lisp
     (convert Reg ValueRegs value_reg)

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:15):

cfallin commented on issue #3807:

OK, great -- added a note in the language reference highlighting the explicit decl, and in the Cranelift-ISLE integration doc. Thanks!

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:15):

fitzgen commented on issue #3807:

Either way, that should be a nicely reported error, not an unwrap panic.

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:16):

cfallin commented on issue #3807:

Either way, that should be a nicely reported error, not an unwrap panic.

Yep, it was already a properly reported error (the unwrap was in the unit-test wrapper), but I added type and term names in this commit.

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:17):

fitzgen commented on issue #3807:

Oh I see it is your code that is doing the unwrap not the ISLE compiler itself, nvm!

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:21):

fitzgen commented on issue #3807:

@avanhatt FWIW, you should be able to get better error messages with source context snippets and stuff (like how rustc provides) if you crib from this code a bit:

• https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/codegen/build.rs#L325
• https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/codegen/build.rs#L417-L444

(It is annoying that it requires this song and dance, and we should improve that at some point, but that's what is needed for the moment.)

Wasmtime GitHub notifications bot (Feb 23 2022 at 20:29):

avanhatt commented on issue #3807:

Great, thanks for the edits + source debugging tip! And sorry for the unwrap confusion.

One other small observation about implicit conversions: one thing you might lose when these are done implicitly is seeing why a special case rewrite is beneficial at a first glance.

For example, in the add-to-sub rule, it's not as obvious anymore that the RHS avoids an extra register use, because visually x and y look the same:

(rule (lower (has_type (fits_in_64 ty) (iadd x (imm12_from_negated_value y))))
      (sub_imm ty x y))

Definitely less important than the convenience this adds, especially with motivating comments, but thought I'd point it out.

Wasmtime GitHub notifications bot (Feb 23 2022 at 21:27):

fitzgen commented on issue #3807:

For example, in the add-to-sub rule, it's not as obvious anymore that the RHS avoids an extra register use, because visually x and y look the same:

Yeah there is certainly a balance to be struck here. For example, with x64's RegMemImm, it may make sense to only have conversions for Reg to RegMemImm and not from memory operands and immediates. This would let us highlight the interesting operands while cutting out noise for the uninteresting common case of registers.

Last updated: Nov 22 2024 at 17:03 UTC