Stream: git-wasmtime

Topic: wasmtime / issue #3679 Improve code generation for floati...

Wasmtime GitHub notifications bot (Jan 11 2022 at 20:16):

alexcrichton commented on issue #3679:

I think this path in lowering was never actually hit due to roughly this block of code. My hope was to eventually reimplement the gen_constant method with an ISLE-based lowering eventually but I figured it may also be pretty tricky to do so.

Wasmtime GitHub notifications bot (Jan 12 2022 at 13:41):

FreddieLiardet commented on issue #3679:

I have added some more tests for simple f32/64const cases.

I think this path in lowering was never actually hit due to roughly this block of code.

Yes I think this is the case.

Wasmtime GitHub notifications bot (Jan 13 2022 at 18:22):

cfallin commented on issue #3679:

I think this path in lowering was never actually hit due to roughly this block of code

Ah, yes, this is the bit I had forgotten... we should never see an F32const/F64const value at the top-level lowering entry point, because either it is used somewhere and rematerialized at every use, or never used and hence dropped by the implicit DCE (use-counting). The existing top-level handlers for F32const/F64const were written before the rematerialize-at-uses change was made, I think.

@FreddieLiardet given this, the change I think doesn't have to touch ISLE at all, since we haven't yet moved constant generation over. Could you:

• Remove the addition to inst.isle (we don't need to define variants of MachInst in the ISLE prelude that we don't use)
• Change the F32const/F64const lowering entry points from implemented_in_isle!() to a panic saying something like "Should never see constant ops at top-level lowering entry point, as constants are rematerialized at use-sites"

Then I think this should be good to merge -- thanks!

Wasmtime GitHub notifications bot (Jan 14 2022 at 10:58):

FreddieLiardet commented on issue #3679:

@cfallin I am unsure about the first point. Is there another place where FpuMoveFPImm instruction should be declared? This instruction is used throughout this change.

Wasmtime GitHub notifications bot (Jan 18 2022 at 03:30):

cfallin commented on issue #3679:

@FreddieLiardet ah, my apologies, I was out-of-sync with our transition: we had previously defined the Inst enum in plain Rust and declared the enum arms we needed separately in ISLE, but I had forgotten that we now generate the enum itself from ISLE. So your diff to add the instruction here is good (modulo the generated-code merge conflict, which you should be able to resolve by rebasing).

Last updated: Nov 22 2024 at 16:03 UTC