Add support for fmsub and fnmsub instructions on x86 #8888
Conversation
github-actions
bot
added
cranelift
There was a problem hiding this comment.
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.
|
Sure, I'll add the runtime tests later, when I have time. |
|
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? |
|
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. |
There was a problem hiding this 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.
There was a problem hiding this comment.
If that's good I will add some more tests of course.
There was a problem hiding this 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).
| ; popq %rbp | ||
| ; retq | ||
|
|
||
| ;; negate first, then load |
There was a problem hiding this comment.
This could generate more efficient core by selecting vfnmsub132ss instead and skipping the negation.
|
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 |
|
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. |
Issue #8602
Allows generating fmsub and fnmsub instructions as mentioned in the issue.