fitzgen opened issue #6055:
Right now we legalize them into a conditional jump to a cold block with a trap intruction early on in the pipeline.
But every backend already has some variant of
MInst.TrapIf
, which is the same astrapnz
, so it would be easy to support in lowering if we wanted to move it there.Benefits:
Not introducing a bunch of extra blocks makes compilation faster for everyone.
It is also the only legalization that currently introduces new blocks, which is something ISLE can't do right now, and therefore is preventing us from porting legalization to ISLE.
Finally it makes it so we could mark
trapnz
as having idempotent side effects which would allow us to GVN them. This would further improve bounds checks on multiple loads of the same dynamic index with differing static offsets for the non-Spectre case (e.g. what is talked about as next steps in https://github.com/bytecodealliance/wasmtime/pull/6031)
fitzgen labeled issue #6055:
Right now we legalize them into a conditional jump to a cold block with a trap intruction early on in the pipeline.
But every backend already has some variant of
MInst.TrapIf
, which is the same astrapnz
, so it would be easy to support in lowering if we wanted to move it there.Benefits:
Not introducing a bunch of extra blocks makes compilation faster for everyone.
It is also the only legalization that currently introduces new blocks, which is something ISLE can't do right now, and therefore is preventing us from porting legalization to ISLE.
Finally it makes it so we could mark
trapnz
as having idempotent side effects which would allow us to GVN them. This would further improve bounds checks on multiple loads of the same dynamic index with differing static offsets for the non-Spectre case (e.g. what is talked about as next steps in https://github.com/bytecodealliance/wasmtime/pull/6031)
fitzgen labeled issue #6055:
Right now we legalize them into a conditional jump to a cold block with a trap intruction early on in the pipeline.
But every backend already has some variant of
MInst.TrapIf
, which is the same astrapnz
, so it would be easy to support in lowering if we wanted to move it there.Benefits:
Not introducing a bunch of extra blocks makes compilation faster for everyone.
It is also the only legalization that currently introduces new blocks, which is something ISLE can't do right now, and therefore is preventing us from porting legalization to ISLE.
Finally it makes it so we could mark
trapnz
as having idempotent side effects which would allow us to GVN them. This would further improve bounds checks on multiple loads of the same dynamic index with differing static offsets for the non-Spectre case (e.g. what is talked about as next steps in https://github.com/bytecodealliance/wasmtime/pull/6031)
github-actions[bot] commented on issue #6055:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen closed issue #6055:
Right now we legalize them into a conditional jump to a cold block with a trap intruction early on in the pipeline.
But every backend already has some variant of
MInst.TrapIf
, which is the same astrapnz
, so it would be easy to support in lowering if we wanted to move it there.Benefits:
Not introducing a bunch of extra blocks makes compilation faster for everyone.
It is also the only legalization that currently introduces new blocks, which is something ISLE can't do right now, and therefore is preventing us from porting legalization to ISLE.
Finally it makes it so we could mark
trapnz
as having idempotent side effects which would allow us to GVN them. This would further improve bounds checks on multiple loads of the same dynamic index with differing static offsets for the non-Spectre case (e.g. what is talked about as next steps in https://github.com/bytecodealliance/wasmtime/pull/6031)
fitzgen commented on issue #6055:
Fixed in #0972
Last updated: Nov 22 2024 at 16:03 UTC