-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[PowerPC] Miscompilation of some ordered comparisons #1014
Comments
Is this fixed now with the fcmp instruction? |
nope. The code generator has always known about seto. The ppc backend doesn't do the right thing with |
If this goes in as a legalize change, could a similar change go in for archs |
That's an interesting approach. Do you suggest a table of supported fp opcodes? |
adding nate so he sees recent comments. |
Looking into this, we need to be able to refer to subregs of virtual condition registers in order to have Consider the two classes of FP comparison we currently get wrong:
This should be codegen'd as:
This should be codegen'd as: Today, we can only emit cror of physical CR regs, since we have to know the exact bit of the subreg |
FWIW, we have vreg subregs now, so this should be implementable. |
We also get SETONE wrong. I'm working on a hacky patch. |
This is (finally!) implemented, Nate, please review: |
There's still tons of fixme's in getPredicateForSetCC referencing this bug. For example: case ISD::SETOEQ: // FIXME: This is incorrect see llvm/llvm-bugzilla-archive#642 . ... |
This should be fixable now that we have subregs for CR ops |
Legalizer can now expand illegal setcc to two setcc anded / ored together. PPC should switch over by adding some setCondCodeAction in PPCISelLowering.cpp. This should fix the bug and eliminate the FIXME's in getPredicateForSetCC. Dale has agreed to take this. :-) |
Shouldn't that be ORd together, not ANDed together? |
Ahhh, my reading comprehension isn't so hot today, you said and / or :) |
patch |
Sorry, didn't see that you hadn't applied the patch. The patch breaks building llvm-gcc because it rejects SETUGT with integer operands. I'll see if I can fix it. |
Assuming SETUGT/SETULT are integer, llvm-gcc builds but the testsuite is worse than before. This needs more work. |
1 similar comment
Assuming SETUGT/SETULT are integer, llvm-gcc builds but the testsuite is worse than before. This needs more work. |
OK, I've fixed up Chris' patch a bit and checked it in. |
Thanks Dale! |
llvm#1014: 既存のtest/MTをlit対応する(grepで文字列を確認2) See merge request a64fx-swpl/llvm-project!19
hipCatch2 instability 1360 - Unit_hipMalloc3DArray_MaxTexture - uint4 (Subprocess killed) 1389 - Unit_hipArray3DCreate_MaxTexture - uint4 (Subprocess killed)
Extended Description
This is a tracking bug so I (or anyone else who is interested) doesn't forget to add support for ordered ops
in the PPC backend.
Since ordered comparisons (by my reading) don't have single-instruction implementations, they should
probably be split into two ops by the legalizer. For example, setole(x,y) should be seto(x,y) & setle(x,y).
-Chris
The text was updated successfully, but these errors were encountered: