Stream: git-wasmtime

Topic: wasmtime / PR #7547 winch: Solidify unreachable code hand...

Wasmtime GitHub notifications bot (Nov 15 2023 at 22:02):

saulecabrera requested elliottt for a review on PR #7547.

Wasmtime GitHub notifications bot (Nov 15 2023 at 22:02):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #7547.

Wasmtime GitHub notifications bot (Nov 15 2023 at 22:02):

saulecabrera opened PR #7547 from saulecabrera:winch-solidify-unreachable-handling to bytecodealliance:main:

This commit solidifies the approach for unreachable code handling in control flow.

Prior to this change, at unconditional jump sites, the compiler would reset the machine stack as well as the value stack. Even though this appoach might seem natural at first, it actually broke several of the invariants that must be met at the end of each contol block, this was specially noticeable with programs that conditionally entered in an unreachable state, like for example

(module
  (func (;0;) (param i32) (result i32)
    local.get 0
    local.get 0
    if (result i32)
      i32.const 1
      return
    else
      i32.const 2
    end
    i32.sub
  )
  (export "main" (func 0))
)

The approach followed in this commit ensures that all the invariants are met and introduces more guardrails around those invariants. In short, instead of resetting the value stack at unconditional jump sites, the value stack handling is deferred until the reachability analysis restores the reachability of the code generation process, ensuring that the value stack contains the exact amount of values expected by the frame where reachability is restored. Given that unconditional jumps reset the machine stack, when the reachability of the code generation process is restored, the SP offset is also restored which should match the size of the value stack.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 15 2023 at 22:06):

saulecabrera updated PR #7547.

Wasmtime GitHub notifications bot (Nov 16 2023 at 00:34):

elliottt submitted PR review:

This looks good to me, thank you for all the comments!

Wasmtime GitHub notifications bot (Nov 16 2023 at 00:34):

elliottt submitted PR review:

This looks good to me, thank you for all the comments!

Wasmtime GitHub notifications bot (Nov 16 2023 at 00:34):

elliottt created PR review comment:

I wonder if it would be worth omitting the offsets (except for jump targets) so that the diff would have shown only the add rsp, 4 instruction going away?

Wasmtime GitHub notifications bot (Nov 16 2023 at 00:34):

elliottt created PR review comment:

This is only called from ensure_stack_state, is it worth making it not pub?

Wasmtime GitHub notifications bot (Nov 16 2023 at 01:14):

saulecabrera updated PR #7547.

Wasmtime GitHub notifications bot (Nov 16 2023 at 01:16):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2023 at 01:16):

saulecabrera created PR review comment:

Fixed, thanks!

Wasmtime GitHub notifications bot (Nov 16 2023 at 01:17):

saulecabrera updated PR #7547.

Wasmtime GitHub notifications bot (Nov 16 2023 at 01:19):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2023 at 01:19):

saulecabrera created PR review comment:

Can you clarify a bit what do you mean with this? More concretely, are you referring to how the disassembly is formatted?

Wasmtime GitHub notifications bot (Nov 16 2023 at 04:54):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2023 at 04:54):

elliottt created PR review comment:

The instructions in the original program from offset 28 on are the same as those in the new program from 24 on, and the only thing that changed was the offsets. If we didn't emit the offsets, or selectively emitted them only for jump targets, the diff would show that the only thing missing was add rsp, 4.

Wasmtime GitHub notifications bot (Nov 16 2023 at 12:27):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2023 at 12:27):

saulecabrera created PR review comment:

Oh right -- sorry for the confusion, I see it now. I think that's a good idea, it it has the potential to reduce verbosity. Are you good if I create a follow up for this? Because If I'm not wrong, this will change a good chunk of the filetests.

Wasmtime GitHub notifications bot (Nov 16 2023 at 17:16):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Nov 16 2023 at 17:16):

elliottt created PR review comment:

Definitely not necessary to fix that in this PR, just something I was thinking would help future reviews :)

Wasmtime GitHub notifications bot (Nov 16 2023 at 18:02):

saulecabrera merged PR #7547.

Last updated: Nov 22 2024 at 16:03 UTC