[GISel] Add more FP opcodes to CSE #123624
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Probably worth adding more FP ops to CSE in this PR. |
|
@llvm/pr-subscribers-llvm-globalisel Author: lialan (lialan) ChangesFull diff: https://github.com/llvm/llvm-project/pull/123624.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp b/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
index 0ac4a8a0aa910b..4e225cccdec65f 100644
--- a/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp
@@ -65,6 +65,7 @@ bool CSEConfigFull::shouldCSEOpc(unsigned Opc) {
case TargetOpcode::G_BUILD_VECTOR:
case TargetOpcode::G_BUILD_VECTOR_TRUNC:
case TargetOpcode::G_SEXT_INREG:
+ case TargetOpcode::G_FDIV:
return true;
}
return false;
diff --git a/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp b/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
index 822707a1f4ed32..8503be09ed5539 100644
--- a/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
+++ b/llvm/unittests/CodeGen/GlobalISel/CSETest.cpp
@@ -9,6 +9,7 @@
#include "GISelMITest.h"
#include "llvm/CodeGen/GlobalISel/CSEInfo.h"
#include "llvm/CodeGen/GlobalISel/CSEMIRBuilder.h"
+#include "llvm/CodeGenTypes/LowLevelType.h"
#include "gtest/gtest.h"
namespace {
@@ -75,6 +76,11 @@ TEST_F(AArch64GISelMITest, TestCSE) {
auto MIBUnmerge2 = CSEB.buildUnmerge({s32, s32}, Copies[0]);
EXPECT_TRUE(&*MIBUnmerge == &*MIBUnmerge2);
+ // Check G_FDIV
+ auto MIBFDiv = CSEB.buildFDiv(s32, Copies[0], Copies[1]);
+ auto MIBFDiv2 = CSEB.buildFDiv(s32, Copies[0], Copies[1]);
+ EXPECT_TRUE(&*MIBFDiv == &*MIBFDiv2);
+
// Check G_BUILD_VECTOR
Register Reg1 = MRI->createGenericVirtualRegister(s32);
Register Reg2 = MRI->createGenericVirtualRegister(s32);
|
arsenm
approved these changes
Jan 21, 2025
|
Should adjust title to cover more opcodes |
arsenm
reviewed
Jan 21, 2025
| case TargetOpcode::G_FMUL: | ||
| case TargetOpcode::G_FDIV: | ||
| case TargetOpcode::G_FNEG: | ||
| case TargetOpcode::G_FABS: |
There was a problem hiding this comment.
The various mins and maxes also
There was a problem hiding this comment.
question: should we fold G_FMINNUM/G_FMANNUM? the result of comparing 0 and -0 could be different.
There was a problem hiding this comment.
Yes. It also couldn't be. We're also fixing the definition to always have the correct ordered zero handling, so it would just be subject to the ordinary nsz flag handling
lialan
force-pushed
the
lialan/cse_fdiv
branch
from
657d01f to
e20a450
Compare
arsenm
reviewed
Jan 21, 2025
arsenm
reviewed
Jan 21, 2025
arsenm
approved these changes
Jan 21, 2025
| EXPECT_TRUE(&*MIBFMinNum == &*MIBFMinNum2); | ||
|
|
||
| MIBFMinNum2.setFlag(MachineInstr::FmNsz); | ||
| EXPECT_FALSE(&*MIBFMinNum == &*MIBFMinNum2); |
There was a problem hiding this comment.
I believe the DAG behavior is to drop the flag and end up with one flagless node
| MIBFMinNum2.clearFlag(MachineInstr::FmNsz); | ||
| EXPECT_TRUE(&*MIBFMinNum == &*MIBFMinNum2); | ||
|
|
||
| auto MIBFMaxNum = CSEB.buildInstr(TargetOpcode::G_FMAXNUM, {s32}, |
There was a problem hiding this comment.
If we don't have wrappers around buildInstr for these, they should be added
arsenm
approved these changes
Jan 21, 2025
lialan
force-pushed
the
lialan/cse_fdiv
branch
from
65cb87e to
b2b8def
Compare
lialan
force-pushed
the
lialan/cse_fdiv
branch
from
b2b8def to
34e0d62
Compare
arsenm
approved these changes
Jan 22, 2025
|
Please bear with me. There seems to have some weird issues with the CI. Going to bisect and see where the culprit is. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/11820 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/16278 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/10672 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/11742 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/11889 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/15405 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/11034 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/11894 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/12039 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/5788 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/10669 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/108/builds/8427 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/8/builds/10136 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/11744 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/5914 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/10111 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/10031 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/8693 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/11429 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/6320 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/125/builds/5122 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/6878 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/39/builds/4088 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/193/builds/5114 Here is the relevant piece of the build log for the reference |
|
Hi, anybody looking at this error already? |
|
@lialan is looking at this now. The fix should be ready soon. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/110/builds/3517 Here is the relevant piece of the build log for the reference |
|
I am going to revert this in the meanwhile. Please re-commit with a fixed version. |
dklimkin
added a commit
to dklimkin/llvm-project
that referenced
this pull request
Jan 22, 2025
This reverts commit 43177b5.
SG, thanks for the help and sorry about that. |
dklimkin
added a commit
that referenced
this pull request
Jan 22, 2025
lialan
added a commit
to lialan/llvm-project
that referenced
this pull request
Jan 22, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/17654 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/16857 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/1951 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/4857 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/20522 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #122724