We were previously lowering to the incorrect instructions for the
setcc DAG node when using the SETUEQ and SETONE floating point
condition codes. I have fixed this by marking the SETONE code
as Expand and letting the SETUNE code be legal. I have also
fixed up the patterns for FCMNE_PPzZZ and FCMNE_PPzZ0 to use
the correct opcode.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
1304 | I'm not near a computer to do decent testing, but should SETUNE be expanded? It seems like it should become a fcmne, if I'm reading the pseudocode correctly. And the issue here is that SETUNE and SETONE were being treated backwards. There are ISel patterns for FCMNE_PPzZZ and FCMNE_PPzZ0 that still try (but will never?) use SETONE. I think they can be using SETUNE though. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
1304 | I was just typing the same thing after running some tests. Yes, we should be able to change the existing patterns and just flip U to O for the two setCondCodeAction calls. |
llvm/test/CodeGen/AArch64/sve-select.ll | ||
---|---|---|
549–551 | Not the fault of this patch just observational but this looks weird now you've fixed the code generation. This comes from D119424 whereby the original oeq is inverted. That patch uses getSetCCInverse which says Return the operation corresponding to !(X op Y). Perhaps I've got my logic wrong but I think !(X oeq Y) => X une Y whereas this output clearly shows we're getting X one Y. What do you think @david-arm is my logic silly? or is there a potential bug to investigate? |
- Removed expansion of SETUNE and fixed up the fcmne patterns to use SETUNE instead of SETONE as they were incorrect before.
I'm not near a computer to do decent testing, but should SETUNE be expanded? It seems like it should become a fcmne, if I'm reading the pseudocode correctly. And the issue here is that SETUNE and SETONE were being treated backwards.
There are ISel patterns for FCMNE_PPzZZ and FCMNE_PPzZ0 that still try (but will never?) use SETONE. I think they can be using SETUNE though.