Skip to content
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

Closed
lattner opened this issue Oct 28, 2005 · 20 comments
Closed

[PowerPC] Miscompilation of some ordered comparisons #1014

lattner opened this issue Oct 28, 2005 · 20 comments
Labels

Comments

@lattner
Copy link
Collaborator

lattner commented Oct 28, 2005

Bugzilla Link 642
Resolution FIXED
Resolved on Nov 09, 2008 01:54
Version 1.3
OS All
Depends On llvm/llvm-bugzilla-archive#1350
CC @nlewycky

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

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 17, 2007

Is this fixed now with the fcmp instruction?

@lattner
Copy link
Collaborator Author

lattner commented Jan 17, 2007

nope. The code generator has always known about seto. The ppc backend doesn't do the right thing with
it yet.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 30, 2007

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?

@lattner
Copy link
Collaborator Author

lattner commented Mar 30, 2007

That's an interesting approach. Do you suggest a table of supported fp opcodes?

@lattner
Copy link
Collaborator Author

lattner commented Mar 30, 2007

adding nate so he sees recent comments.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 28, 2007

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}

  1. 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.

@lattner
Copy link
Collaborator Author

lattner commented Sep 26, 2007

FWIW, we have vreg subregs now, so this should be implementable.

@lattner
Copy link
Collaborator Author

lattner commented Jan 8, 2008

We also get SETONE wrong. I'm working on a hacky patch.

@lattner
Copy link
Collaborator Author

lattner commented Jan 8, 2008

This is (finally!) implemented, Nate, please review:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080107/056957.html

@nlewycky
Copy link
Contributor

nlewycky commented Mar 3, 2008

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 .
case ISD::SETUEQ:
case ISD::SETEQ: return PPC::PRED_EQ;
case ISD::SETONE: // FIXME: This is incorrect see llvm/llvm-bugzilla-archive#642 .
case ISD::SETUNE:

...

@lattner
Copy link
Collaborator Author

lattner commented Mar 30, 2008

This should be fixable now that we have subregs for CR ops

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 16, 2008

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. :-)

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 16, 2008

Shouldn't that be ORd together, not ANDed together?

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 16, 2008

Ahhh, my reading comprehension isn't so hot today, you said and / or :)

@lattner
Copy link
Collaborator Author

lattner commented Oct 30, 2008

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.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 7, 2008

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.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 7, 2008

Assuming SETUGT/SETULT are integer, llvm-gcc builds but the testsuite is worse than before. This needs more work.

1 similar comment
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 7, 2008

Assuming SETUGT/SETULT are integer, llvm-gcc builds but the testsuite is worse than before. This needs more work.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 8, 2008

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.

@lattner
Copy link
Collaborator Author

lattner commented Nov 9, 2008

Thanks Dale!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 3, 2021
kitano-metro pushed a commit to RIKEN-RCCS/llvm-project that referenced this issue Dec 2, 2022
kitano-metro pushed a commit to RIKEN-RCCS/llvm-project that referenced this issue Dec 2, 2022
kitano-metro pushed a commit to RIKEN-RCCS/llvm-project that referenced this issue Dec 2, 2022
kitano-metro pushed a commit to RIKEN-RCCS/llvm-project that referenced this issue Dec 2, 2022
kitano-metro pushed a commit to RIKEN-RCCS/llvm-project that referenced this issue Dec 2, 2022
llvm#1014: 既存のtest/MTをlit対応する(grepで文字列を確認2)

See merge request a64fx-swpl/llvm-project!19
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants