Stream: git-wasmtime

Topic: wasmtime / PR #8888 Add support for fmsub and fnmsub inst...


view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2024 at 21:14):

UnlimitedHummus opened PR #8888 from UnlimitedHummus:main to bytecodealliance:main:

Issue #8602

Allows generating fmsub and fnmsub instructions as mentioned in the issue.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2024 at 21:14):

UnlimitedHummus requested fitzgen for a review on PR #8888.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 30 2024 at 21:14):

UnlimitedHummus requested wasmtime-compiler-reviewers for a review on PR #8888.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2024 at 19:37):

fitzgen submitted PR review:

Thanks! Overall looks good to me, modulo the one comment below.

Can you also add some run tests in addition to the compile tests, to make sure that the runtime behavior is as-expected as well? Since FMA is not quite the same across architectures, it may make sense to put these run tests under cranelift/filetests/filetests/isa/x64 as well.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2024 at 19:37):

fitzgen submitted PR review:

Thanks! Overall looks good to me, modulo the one comment below.

Can you also add some run tests in addition to the compile tests, to make sure that the runtime behavior is as-expected as well? Since FMA is not quite the same across architectures, it may make sense to put these run tests under cranelift/filetests/filetests/isa/x64 as well.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2024 at 19:37):

fitzgen created PR review comment:

I forget what the details of our lowering and ISLE conversions are these days, but does this need to guard on sinkable loads? Or will it force things to registers as written? (In which case the comment should be updated)

cc @jameysharp, who I think would know the answer to this question.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2024 at 23:15):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 01 2024 at 23:15):

jameysharp created PR review comment:

I'm not actually sure. But I see that this is identical to the existing lowering rules above aside from replacing add with sub and so on. So it's probably fine?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 06:08):

UnlimitedHummus submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 06:08):

UnlimitedHummus created PR review comment:

I did just work off of the rules for the add instructions. However, its probably related to the semantics of z matching into XmmMem in the rule for x64vfmsub213. I didn't look much into how that works, but the first two test cases actually test this rule both in the case of passing a value in a register or a sinkable load. There, the load is definitely sunk.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 06:10):

UnlimitedHummus commented on PR #8888:

Sure, I'll add the runtime tests later, when I have time.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 07:33):

afonso360 commented on PR #8888:

We already have some run tests that run on all arch's here (1, 2, 3). None of them test load sinking, so that would be a nice addition.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 07:35):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 07:35):

afonso360 created PR review comment:

I forget what the details of our lowering and ISLE conversions are these days, but does this need to guard on sinkable loads?

We do some checks when converting from a Value into XmmMem where we force the value into a register if it's not eligible for being sunk (i.e. has multiple uses), so this looks okay to me.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 08:16):

afonso360 submitted PR review:

This LGTM! I double checked the new instruction encodings against the intel manual and they all check out. Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 15:56):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 15:56):

fitzgen created PR review comment:

Thanks for the clarification @afonso360 and @jameysharp!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 20:01):

UnlimitedHummus commented on PR #8888:

To add runtime tests for load sinking, I would just duplicate some existing testcases and add loads to them. This doesn't test that the load is actually sunk, but rather that the results are still correct with load sinking. Is that the intention? Or am I missing some different type of testcase I could write?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 02 2024 at 20:28):

fitzgen commented on PR #8888:

That is a good thing to test, but generally I was just asking for runtests that check that the new lowerings that turn multiple clif instructions into a single machine instruction produce the same values that interpreting the individual clif instructions would.

See for example

https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/filetests/filetests/runtests/iabs.clif#L1-L18

although we only care about the interpreter and x86-64 here.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 19:17):

UnlimitedHummus updated PR #8888.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 19:19):

UnlimitedHummus submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 19:19):

UnlimitedHummus created PR review comment:

Is this what you had in mind? I didn't find a better way to test this, other than to store the value on the stack and then load it from there again.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 19:22):

UnlimitedHummus submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 19:22):

UnlimitedHummus created PR review comment:

If that's good I will add some more tests of course.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 20:48):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 03 2024 at 20:48):

afonso360 created PR review comment:

Yeah! Just something to ensure that the instruction gets emitted and runs properly. I wouldn't worry about trying to get all of the variations running, that might be too much (In my opinion).

view this post on Zulip Wasmtime GitHub notifications bot (Jul 12 2024 at 22:50):

UnlimitedHummus updated PR #8888.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 12 2024 at 23:00):

UnlimitedHummus submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 12 2024 at 23:00):

UnlimitedHummus created PR review comment:

This could generate more efficient core by selecting vfnmsub132ss instead and skipping the negation.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 12 2024 at 23:02):

UnlimitedHummus commented on PR #8888:

I added some tests, but also identified an optimization I missed. This only affects the case where the negation happens before the load. I think the order of the two shouldn't matter. I still need to look into how to write rules to handle that case.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 12 2024 at 23:50):

jameysharp commented on PR #8888:

I added some tests, but also identified an optimization I missed. This only affects the case where the negation happens before the load. I think the order of the two shouldn't matter. I still need to look into how to write rules to handle that case.

This PR is already a nice improvement even if it doesn't optimize every case, so: If you want to dig into that you're welcome to, but I'd also be happy to merge this as-is. If you decide not to fix it right now, then it would be great to open a new issue describing what's not working, and include a link to that issue in the comments on the test that illustrates the problem.

That said, this is failing in CI right now. The error looks weird to me and I'm not sure whether you just need to re-run CRANELIFT_TEST_BLESS=1 cargo test -p cranelift-tools --test filetests or if there's something stranger going on, so please let us know if you have any trouble.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 10:52):

UnlimitedHummus updated PR #8888.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 11:25):

UnlimitedHummus updated PR #8888.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 11:38):

UnlimitedHummus commented on PR #8888:

CI failing was due to me annotating a copy of the expected output. I moved my comment before the testcase, which fixed that. I tried adding some rules to improve code generation for this case yesterday, but that didn't work. So I'd have to take a closer look at ISLE to see why it doesn't generate the right code. Not sure when I'll get around to that so I opened an issue describing the missed optimization like you suggested.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 13 2024 at 13:46):

afonso360 merged PR #8888.


Last updated: Dec 23 2024 at 12:05 UTC