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 thanwith_flags_1
, returning aValueRegs
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 bewith_flags_return_producer
,with_flags_return_consumer
, ..., but maybe there are better names?
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>
fitzgen commented on issue #3585:
We should maybe name
with_flags_*
more descriptively... slightly lame suggestions could bewith_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 regswith_flags_producer
: gives the producer resultwith_flags_consumer
: gives the consumer resultwith_flags_both
: gives(producer, consumer)
?
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.
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 wheniadd_ifcout
is used by atrapif
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 wantiflags
-type values to be replaced by bools and pattern-matching, I don't think we want to fully reify theiflags
in ISLE. Probably the best option is to just return a freshly-allocated second temp for the flags for now; anything that usesiflags
will not be handled by ISLE so we will not be in danger of actually using it.
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: Dec 23 2024 at 12:05 UTC