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))
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
orput_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?
avanhatt commented on issue #3807:
@cfallin ah, it was a silly mistake, I misinterpreted the
convert
statement as _replacing_ thedecl
/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)
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!
fitzgen commented on issue #3807:
Either way, that should be a nicely reported error, not an
unwrap
panic.
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.
fitzgen commented on issue #3807:
Oh I see it is your code that is doing the
unwrap
not the ISLE compiler itself, nvm!
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.)
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
andy
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.
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
andy
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 forReg
toRegMemImm
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: Dec 23 2024 at 12:05 UTC