LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 642 - [PowerPC] Miscompilation of some ordered comparisons
Summary: [PowerPC] Miscompilation of some ordered comparisons
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: PowerPC (show other bugs)
Version: 1.3
Hardware: All All
: P normal
Assignee: Dale Johannesen
URL:
Keywords: miscompilation
Depends on: 1350
Blocks:
  Show dependency tree
 
Reported: 2005-10-28 15:45 PDT by Chris Lattner
Modified: 2008-11-09 01:54 PST (History)
5 users (show)

See Also:
Fixed By Commit(s):


Attachments
patch (3.27 KB, patch)
2008-10-30 00:43 PDT, Chris Lattner
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lattner 2005-10-28 15:45:18 PDT
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
Comment 1 Reid Spencer 2007-01-17 13:35:50 PST
Is this fixed now with the fcmp instruction?
Comment 2 Chris Lattner 2007-01-17 13:41:19 PST
nope.  The code generator has always known about seto.  The ppc backend doesn't do the right thing with 
it yet.
Comment 3 Andrew Lenharth 2007-03-29 23:09:53 PDT
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?
Comment 4 Chris Lattner 2007-03-29 23:11:49 PDT
That's an interesting approach.  Do you suggest a table of supported fp opcodes?
Comment 5 Chris Lattner 2007-03-29 23:12:29 PDT
adding nate so he sees recent comments.
Comment 6 Nate Begeman 2007-04-28 01:17:47 PDT
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.
Comment 7 Chris Lattner 2007-09-26 00:46:40 PDT
FWIW, we have vreg subregs now, so this should be implementable.
Comment 8 Chris Lattner 2008-01-07 23:44:11 PST
We also get SETONE wrong.  I'm working on a hacky patch.
Comment 9 Chris Lattner 2008-01-08 00:47:46 PST
This is (finally!) implemented, Nate, please review:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080107/056957.html
Comment 10 Nick Lewycky 2008-03-02 21:29:41 PST
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:
 
...
Comment 11 Chris Lattner 2008-03-30 14:43:07 PDT
This should be fixable now that we have subregs for CR ops
Comment 12 Evan Cheng 2008-10-16 13:15:57 PDT
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. :-)
Comment 13 Nate Begeman 2008-10-16 13:29:32 PDT
Shouldn't that be ORd together, not ANDed together?
Comment 14 Nate Begeman 2008-10-16 13:30:09 PDT
Ahhh, my reading comprehension isn't so hot today, you said and / or :)
Comment 15 Chris Lattner 2008-10-30 00:43:55 PDT
Created attachment 2158 [details]
patch

This attached patch seems to work for me in my limited touch testing, but I haven't run full tests with it and won't have time to in the near future.  I'd appreciate it if someone could verify that it works and finally close off this old bug.
Comment 16 Dale Johannesen 2008-11-07 13:29:11 PST
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.


Comment 17 Dale Johannesen 2008-11-07 15:07:10 PST
Assuming SETUGT/SETULT are integer, llvm-gcc builds but the testsuite is worse than before.  This needs more work.

Comment 18 Dale Johannesen 2008-11-07 15:07:20 PST
Assuming SETUGT/SETULT are integer, llvm-gcc builds but the testsuite is worse than before.  This needs more work.
Comment 19 Dale Johannesen 2008-11-07 17:00:52 PST
OK, I've fixed up Chris' patch a bit and checked it in.
http://llvm.org/viewvc/llvm-project?view=rev&revision=58871
That makes all tests in the gcc testsuite that are obviously about testing FP comparisons pass, at least (and unordered is definitely exercised).  So I think this is fixed.


Comment 20 Chris Lattner 2008-11-09 01:54:38 PST
Thanks Dale!