Stream: git-wasmtime

Topic: wasmtime / PR #7114 Cranelift: return programmatic error ...

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:27):

cfallin opened PR #7114 from cfallin:handle-code-too-large to bytecodealliance:main:

Cranelift currently has a limit of 2^21 vregs per function body in VCode. This is a consequence of (performance-motivated) bitpacking of Operands into u32s in regalloc2.

As a result of this, it is possible to produce a function body that will fail to compile by running out of vreg temporaries during lowering. Currently, this results in a panic. Ideally, we would propagate the error upward and return it programmatically.

This PR does that, with a "deferred error" mechanism. A cleaner solution would be to properly thread the Result types through all layers of lowering. However, that would require supporting Results in ISLE, and that is a deeper language-design and islec-hacking question that I think we can tackle later if we continue to see motivating cases.

The deferral works by returning a valid but bogus ValueRegs to the lowering rule, but storing the error and checking for it in the toplevel lowering loop. (Note that we have to return a bogus v0 rather than VReg::invalid(), because the latter causes the ValueRegs to think there is no register provided.)

This PR also includes a test at the Wasmtime level. Note that it takes ~22s to run on my (relatively fast) laptop, because it has to run until it runs out of VRegs in a debug build of the compiler. We could remove the test if we feel we're otherwise confident.

Thanks to Venkkatesh Sekar for reporting this issue! The integration test uses one of the example inputs from the report.

<!--
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 (Sep 29 2023 at 18:27):

cfallin requested wasmtime-compiler-reviewers for a review on PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:27):

cfallin requested abrown for a review on PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:27):

cfallin requested wasmtime-core-reviewers for a review on PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:27):

cfallin requested alexcrichton for a review on PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:32):

alexcrichton submitted PR review:

Looks good to me!

Would it perhaps be worth handling other callers of vregs.alloc as well? There's a number of other vregs.alloc(...).unwrap() through the ABI code. Additionally there's preexisting error handling for vregs.alloc(...)? and could that use this same pattern as well? It might be relatively simplifying to assume that vreg allocation always succeeds and only at the end is the global error checked to see if allocation actually failed.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:37):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:37):

fitzgen created PR review comment:

Does it make sense to check this after lowering each block rather than only after lowering all blocks? Not saying this isn't sufficient, just wondering whether it makes sense to potentially notice these errors and propagate them earlier.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:37):

fitzgen created PR review comment:

Can we name this bogus_placeholder or something instead of "invalid"? Because we already have invalid registers and they have different semantics from this, as you already noted.

Wasmtime GitHub notifications bot (Sep 29 2023 at 18:37):

fitzgen created PR review comment:

Could this method return a proper Result and then we have another alloc_tmp_deferred_error method that does the stuff-the-error-on-the-side bits? Then we could propagate the result normally from most callers and just the ISLE callers could use the deferral mechanism.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:40):

cfallin updated PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:44):

cfallin updated PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:48):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:49):

cfallin created PR review comment:

Good point, lifted this into the loop.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:49):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:50):

cfallin created PR review comment:

I thought about this a bit but it seemed simpler to me to keep the one entry point -- as a method on Lower it's the one way to allocate temps during the main lowering loop, and we check during that loop for errors.

Wasmtime GitHub notifications bot (Sep 29 2023 at 19:54):

alexcrichton submitted PR review:

Looks good :+1:

As a final confirmation though since I'm not as familiar with the ABI code as you, are deferred errors generated during ABI business guaranteed to get handled by checking after basic-block emission?

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:02):

cfallin updated PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 20:05):

cfallin has enabled auto merge for PR #7114.

Wasmtime GitHub notifications bot (Sep 29 2023 at 21:10):

cfallin merged PR #7114.

Last updated: Nov 22 2024 at 17:03 UTC