Bugzilla – Bug 642
[PowerPC] Miscompilation of some ordered comparisons
Last modified: 2008-03-30 14:43:07
You need to log in before you can comment on or make changes to this bug.
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
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 it yet.
If this goes in as a legalize change, could a similar change go in for archs which need to issue an setu | setxx to get unordored comparisons?
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 decent codegen. Consider the two classes of FP comparison we currently get wrong: 1. Unordered + {LT, GT, EQ}. This should be codegen'd as: fcmpu crA, fA, fB cror crA.un, crA.un, crA.{lt,gt,eq} 2. Ordered + {LE, GE} This should be codegen'd as: fcmpu crA, fA, fB cror crA.eq, crA.eq, crA.{lt,gt} Today, we can only emit cror of physical CR regs, since we have to know the exact bit of the subreg when we create the node. This works ok for SETCC today, where we force the comparison into CR7 (which would be unnecessary with subreg support). However, for BR_CC or SELET_CC, it currently forces us to generate incorrect code.
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: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080107/056957.html
There's still tons of fixme's in getPredicateForSetCC referencing this bug. For example: case ISD::SETOEQ: // FIXME: This is incorrect see PR642. case ISD::SETUEQ: case ISD::SETEQ: return PPC::PRED_EQ; case ISD::SETONE: // FIXME: This is incorrect see PR642. case ISD::SETUNE: ...
This should be fixable now that we have subregs for CR ops