search

avanhatt / wasmtime

Standalone JIT-style runtime for WebAssembly, using Cranelift
https://wasmtime.dev/
Apache License 2.0
0 stars 1 forks source link

Fix type inference for enum variants #73

Closed mmcloughlin closed 1 year ago

mmcloughlin commented 1 year ago

This PR proposes a fix for the type inference problem that was preventing verification of the Amode lowering with the expression (Amode.ImmRegRegShift offset x y 0 flags).

https://github.com/avanhatt/wasmtime/blob/6e0a601361ac92db65de1413afab97e036cbed98/cranelift/codegen/src/isa/x64/inst.isle#L1130-L1131

The specific problem was a failure to deduce the type for the shift argument (set to the constant 0). I believe the root cause was with add_isle_constraints which was only considering declarations, not enum variants. I saw that the Sema layer in ISLE handles this already, so both declarations and enum variants are merged in the TermEnv. Therefore this PR changes add_isle_constraints to operate on Sema Terms rather than AST Decls. This has the (I think) nice side effect that type inference no longer uses any AST types. That also meant that the change had a bit of a larger blast radius, with various function signatures changing etc.

mmcloughlin commented 1 year ago

Please advise on how to test this PR.

avanhatt commented 1 year ago

Please advise on how to test this PR.

The integration rule tests exercise this code (including for specific types), so we have some assurance that this doesn't break everything. But feel free to add more unit tests as you go, too.

mmcloughlin commented 1 year ago

The integration rule tests exercise this code (including for specific types)

Yes, I did rerun the test suite on this PR too.

It would be nice to add type-inference specific tests. Perhaps I'll look into that.