Stream: git-wasmtime

Topic: wasmtime / issue #3585 x64: assert that temporary and des...

Wasmtime GitHub notifications bot (Dec 07 2021 at 19:51):

cfallin commented on issue #3585:

Thanks @abrown! cc @fitzgen: context is that I was helping Andrew debug some ISLE lowerings and we hit a footgun where he was using with_flags rather than with_flags_1, returning a ValueRegs of zero registers, hence this new assertion. From that,

• We should have this assertion, and we should fix the assertion failure (I can take a look maybe tomorrow if someone else doesn't get there first)
• We should maybe name with_flags_* more descriptively... slightly lame suggestions could be with_flags_return_producer, with_flags_return_consumer, ..., but maybe there are better names?

Wasmtime GitHub notifications bot (Dec 07 2021 at 20:04):

github-actions[bot] commented on issue #3585:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "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>

Wasmtime GitHub notifications bot (Dec 07 2021 at 23:38):

fitzgen commented on issue #3585:

We should maybe name with_flags_* more descriptively... slightly lame suggestions could be with_flags_return_producer, with_flags_return_consumer, ..., but maybe there are better names?

Maybe without the "return" so it is

• with_flags_none: gives zero regs
• with_flags_producer: gives the producer result
• with_flags_consumer: gives the consumer result
• with_flags_both: gives (producer, consumer)

?

Wasmtime GitHub notifications bot (Dec 08 2021 at 19:36):

cfallin commented on issue #3585:

It looks like an issue with iadd_ifcout; the lowering only returns the add result, but it's supposed to be both the add result and the carry flag.

I pushed an update to the assertion to the same branch in my fork (it was asserting just for one dst, not for all, but it's still a mismatch). Will look at the lowering, maybe it's simple to fix, otherwise we can just back out iadd_ifcout from ISLE for now to allow the release, I think.

Wasmtime GitHub notifications bot (Dec 08 2021 at 19:37):

cfallin commented on issue #3585:

Further interesting twists: iadd_ifcout in the pre-ISLE world also wrote only to its first output (the add result) when lowered by itself. We have a separate pattern-match when iadd_ifcout is used by a trapif to explicltly do the right thing. As I recall, this special-casing is just enough to get the dynamic heap checks to work, and since we eventually want iflags-type values to be replaced by bools and pattern-matching, I don't think we want to fully reify the iflags in ISLE. Probably the best option is to just return a freshly-allocated second temp for the flags for now; anything that uses iflags will not be handled by ISLE so we will not be in danger of actually using it.

Wasmtime GitHub notifications bot (Dec 08 2021 at 19:47):

cfallin commented on issue #3585:

I pushed a fix to the iadd_ifcout lowering here; please feel free to cherry-pick this and the assertion fix!

Last updated: Nov 22 2024 at 16:03 UTC