Wasmtime GitHub notifications bot (Jun 30 2024 at 21:14): 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:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Jun 30 2024 at 21:14):UnlimitedHummus requested fitzgen for a review on PR #8888.
Wasmtime GitHub notifications bot (Jun 30 2024 at 21:14):UnlimitedHummus requested wasmtime-compiler-reviewers for a review on PR #8888.
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/x64as well.
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/x64as well.
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.
Wasmtime GitHub notifications bot (Jul 01 2024 at 23:15):jameysharp submitted PR review.
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
addwithsuband so on. So it's probably fine?
Wasmtime GitHub notifications bot (Jul 02 2024 at 06:08):UnlimitedHummus submitted PR review.
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
addinstructions. However, its probably related to the semantics ofzmatching intoXmmMemin the rule forx64vfmsub213. 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.
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.
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.
Wasmtime GitHub notifications bot (Jul 02 2024 at 07:35):afonso360 submitted PR review.
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
ValueintoXmmMemwhere 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.
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!
Wasmtime GitHub notifications bot (Jul 02 2024 at 15:56):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Jul 02 2024 at 15:56):fitzgen created PR review comment:
Thanks for the clarification @afonso360 and @jameysharp!
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?
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
although we only care about the interpreter and x86-64 here.
Wasmtime GitHub notifications bot (Jul 03 2024 at 19:17):UnlimitedHummus updated PR #8888.
Wasmtime GitHub notifications bot (Jul 03 2024 at 19:19):UnlimitedHummus submitted PR review.
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.
Wasmtime GitHub notifications bot (Jul 03 2024 at 19:22):UnlimitedHummus submitted PR review.
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.
Wasmtime GitHub notifications bot (Jul 03 2024 at 20:48):afonso360 submitted PR review.
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).
Wasmtime GitHub notifications bot (Jul 12 2024 at 22:50):UnlimitedHummus updated PR #8888.
Wasmtime GitHub notifications bot (Jul 12 2024 at 23:00):UnlimitedHummus submitted PR review.
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.
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.
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 filetestsor if there's something stranger going on, so please let us know if you have any trouble.
Wasmtime GitHub notifications bot (Jul 13 2024 at 10:52):UnlimitedHummus updated PR #8888.
Wasmtime GitHub notifications bot (Jul 13 2024 at 11:25):UnlimitedHummus updated PR #8888.
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.
Wasmtime GitHub notifications bot (Jul 13 2024 at 13:46):afonso360 merged PR #8888.
Last updated: Oct 23 2024 at 20:03 UTC