Stream: git-wasmtime

Topic: wasmtime / PR #4254 X64: port the rest of icmp to ISLE

Wasmtime GitHub notifications bot (Jun 10 2022 at 03:04):

elliottt opened PR #4254 from trevor/icmp-isle-port to main:

• Finish migrating icmp to ISLE
• Update test output

<!--

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.
-->

Wasmtime GitHub notifications bot (Jun 10 2022 at 04:31):

elliottt has marked PR #4254 as ready for review.

Wasmtime GitHub notifications bot (Jun 10 2022 at 05:47):

elliottt updated PR #4254 from trevor/icmp-isle-port to main.

Wasmtime GitHub notifications bot (Jun 10 2022 at 05:53):

elliottt created PR review comment:

Is the setnz in the original necessary here; it seems like andq with an immediate $1 should be enough?

Wasmtime GitHub notifications bot (Jun 10 2022 at 05:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jun 10 2022 at 05:53):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Jun 13 2022 at 18:40):

elliottt requested cfallin for a review on PR #4254.

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin created PR review comment:

Could we add a comment here describing what without_eq means (probably just summarizing the doc comment for IntCC::without_equal() and/or pointing to it)?

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin created PR review comment:

I am wondering now if this and is necessary: it's masking high bits off the result of the or for res above, but in the end this is a combination of setcc results, which are all 0/1...

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin created PR review comment:

emit_cmp remains used because of the branch cases below, right?

It's worth thinking a bit about how we're going to factor this apart so branches can use the icmp logic as well (perhaps it becomes a helper that returns "result in rflags with this CC if possible, otherwise this 0/1 bool in register"), but that's not strictly necessary for this PR. If you're looking for a next thing to do, sketching out the conditional branch lowering (s390x already uses ISLE for this, as a reference point) would be good while this is in your cache :-)

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin created PR review comment:

Likewise here, a quick summary or pointer would be helpful.

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:04):

cfallin created PR review comment:

Indeed, it seems redundant. I suspect it might have been an artifact of lowering this in separate "generate the flags" and "use in trueif, brif, ..." halves, where the and generates flags (and also incidentally produces the 0/1 output directly).

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:53):

elliottt created PR review comment:

Great point! I'll experiment with returning the result of the or instead :)

Wasmtime GitHub notifications bot (Jun 13 2022 at 19:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2022 at 20:20):

elliottt created PR review comment:

As we discussed in a DM, as the setcc instructions only set the lowest byte and leave the upper 56 bits of the register preserved, the and will be necessary.

Wasmtime GitHub notifications bot (Jun 13 2022 at 20:20):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2022 at 20:20):

elliottt updated PR #4254 from trevor/icmp-isle-port to main.

Wasmtime GitHub notifications bot (Jun 13 2022 at 20:28):

elliottt updated PR #4254 from trevor/icmp-isle-port to main.

Wasmtime GitHub notifications bot (Jun 13 2022 at 21:54):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 13 2022 at 23:34):

elliottt merged PR #4254.

Last updated: Nov 22 2024 at 16:03 UTC