elliottt opened PR #4545 from trevor/x64-lower-trapif
to main
:
- Add a ProducesBool type for capturing comparisons
- Migrate trapif to ISLE
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt edited PR #4545 from trevor/x64-lower-trapif
to main
:
Migrate the
trapif
instruction to ISLE, and extract the lowering oficmp
for scalars to a utility function. Theicmp
extraction will be helpful for future instruction migrations, as it mirrors theemit_cmp
instruction in the rust implementation.<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt edited PR #4545 from trevor/x64-lower-trapif
to main
:
Migrate the
trapif
instruction to ISLE, and extract the lowering oficmp
for scalars to a utility function. Theicmp
extraction will be helpful for future instruction migrations, as it mirrors theemit_cmp
instruction in the rust implementation.I'd recommend reviewing by commit, and turning off whitespace in the diff.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt updated PR #4545 from trevor/x64-lower-trapif
to main
.
elliottt updated PR #4545 from trevor/x64-lower-trapif
to main
.
elliottt edited PR #4545 from trevor/x64-lower-trapif
to main
:
Migrate the
trapif
instruction to ISLE, and extract the lowering oficmp
for scalars to a utility function. Theicmp
extraction will be helpful for future instruction migrations, as its uses mirror the existingemit_cmp
instruction in the rust implementation.I'd recommend reviewing by commit, and turning off whitespace in the diff.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
elliottt updated PR #4545 from trevor/x64-lower-trapif
to main
.
elliottt requested cfallin for a review on PR #4545.
elliottt has marked PR #4545 as ready for review.
elliottt updated PR #4545 from trevor/x64-lower-trapif
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Hmm -- I'm not sure about this: we're matching that
iadd_ifcout
is our input, but we don't actually use the input, rather we implicitly depend on it clobbering flags. This then breaks our producer-consumer coupling as well, necessitating theemit_consumer
helper, which feels pretty unsafe to me.The most immediate issue is that if the
iadd_ifcout
isn't otherwise used, it won't actually be lowered, and then we're going to use bogus flags to do our trap. That seems like the biggest issue. I suspect this case simply isn't covered in our existing tests.More broadly, we want to remove
flags
types competely eventually, because they're pretty error-prone (see #3249). But until then, we should work on doing something safer (at least correct, ideally more elegant as well!).What about this: we write a constructor that takes the flags value (so in the pattern, capture it with
flags @ (iadd_ifcout _ _)
), and produces aProducesFlags.AlreadyExistingFlags
(that contains no actual inst), and handle that inwith_flags
. This ctor will need to call out to an external constructor to bump the refcount on the iadd_ifcoutas well (normally
put_in_regsdoes this in an implicit converter from
Valueto
Reg). Let's add a test where the
iadd_ifcout` is used only for its flags, and make sure it lowers properly.There's also a relevant comment near the lowering rules for
iadd_ifcout
that should be updated I think.Thoughts?
elliottt updated PR #4545 from trevor/x64-lower-trapif
to main
.
Last updated: Dec 23 2024 at 13:07 UTC