[RISCV] Use Root instead of N throughout the worklist loop in combineBinOp_VLToVWBinOp_VL. #99416
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
…BinOp_VLToVWBinOp_VL. We were only checking that the node from the worklist is a supported root. We weren't checking the strategy or any of its operands unless it was the original node. For any other node, we just rechecked the original node's strategy and operands. The effect of this is that we don't do all of the transformations at once. Instead, when there were multiple possible nodes to transform we would only do them as each node was visited by the main DAG combine worklist. I'm not sure if there's a way to construct a test to show this or not. I'll keep trying.
|
@llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesWe were only checking that the node from the worklist is a supported root. We weren't checking the strategy or any of its operands unless it was the original node. For any other node, we just rechecked the original node's strategy and operands. The effect of this is that we don't do all of the transformations at once. Instead, when there were multiple possible nodes to transform we would only do them as each node was visited by the main DAG combine worklist. I'm not sure if there's a way to construct a test to show this or not. I'll keep trying. Full diff: https://github.com/llvm/llvm-project/pull/99416.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 21193ebe1eb94..e938454b8e642 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -14998,8 +14998,8 @@ static SDValue combineBinOp_VLToVWBinOp_VL(SDNode *N,
if (!NodeExtensionHelper::isSupportedRoot(Root, Subtarget))
return SDValue();
- NodeExtensionHelper LHS(N, 0, DAG, Subtarget);
- NodeExtensionHelper RHS(N, 1, DAG, Subtarget);
+ NodeExtensionHelper LHS(Root, 0, DAG, Subtarget);
+ NodeExtensionHelper RHS(Root, 1, DAG, Subtarget);
auto AppendUsersIfNeeded = [&Worklist,
&Inserted](const NodeExtensionHelper &Op) {
if (Op.needToPromoteOtherUsers()) {
@@ -15016,18 +15016,18 @@ static SDValue combineBinOp_VLToVWBinOp_VL(SDNode *N,
return SDValue();
SmallVector<NodeExtensionHelper::CombineToTry> FoldingStrategies =
- NodeExtensionHelper::getSupportedFoldings(N);
+ NodeExtensionHelper::getSupportedFoldings(Root);
assert(!FoldingStrategies.empty() && "Nothing to be folded");
bool Matched = false;
for (int Attempt = 0;
- (Attempt != 1 + NodeExtensionHelper::isCommutative(N)) && !Matched;
+ (Attempt != 1 + NodeExtensionHelper::isCommutative(Root)) && !Matched;
++Attempt) {
for (NodeExtensionHelper::CombineToTry FoldingStrategy :
FoldingStrategies) {
std::optional<CombineResult> Res =
- FoldingStrategy(N, LHS, RHS, DAG, Subtarget);
+ FoldingStrategy(Root, LHS, RHS, DAG, Subtarget);
if (Res) {
Matched = true;
CombinesToApply.push_back(*Res);
|
qcolombet
approved these changes
Jul 18, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/1945 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/1979 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/2746 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/1957 Here is the relevant piece of the build log for the reference: |
topperc
added a commit
that referenced
this pull request
Jul 18, 2024
… in combineBinOp_VLToVWBinOp_VL. (#99416)" With correct test update. Original message: We were only checking that the node from the worklist is a supported root. We weren't checking the strategy or any of its operands unless it was the original node. For any other node, we just rechecked the original node's strategy and operands. The effect of this is that we don't do all of the transformations at once. Instead, when there were multiple possible nodes to transform we would only do them as each node was visited by the main DAG combine worklist. The test shows a case where we widened an instruction without removing all of the uses of the vsext. The sext is shared by one node that shares another sext node with the root another node that doesn't share anything with the root.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/298 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/3421 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/16/builds/1964 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/945 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/60/builds/2834 Here is the relevant piece of the build log for the reference: |
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…BinOp_VLToVWBinOp_VL. (#99416) We were only checking that the node from the worklist is a supported root. We weren't checking the strategy or any of its operands unless it was the original node. For any other node, we just rechecked the original node's strategy and operands. The effect of this is that we don't do all of the transformations at once. Instead, when there were multiple possible nodes to transform we would only do them as each node was visited by the main DAG combine worklist. The test shows a case where we widened an instruction without removing all of the uses of the vsext. The sext is shared by one node that shares another sext node with the root another node that doesn't share anything with the root.
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
… combineBinOp_VLToVWBinOp_VL. (#99416)" Summary: This reverts commit 0c4023a. I messed up re-generating the test after the change. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250829
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
… in combineBinOp_VLToVWBinOp_VL. (#99416)" Summary: With correct test update. Original message: We were only checking that the node from the worklist is a supported root. We weren't checking the strategy or any of its operands unless it was the original node. For any other node, we just rechecked the original node's strategy and operands. The effect of this is that we don't do all of the transformations at once. Instead, when there were multiple possible nodes to transform we would only do them as each node was visited by the main DAG combine worklist. The test shows a case where we widened an instruction without removing all of the uses of the vsext. The sext is shared by one node that shares another sext node with the root another node that doesn't share anything with the root. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250791
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.
We were only checking that the node from the worklist is a supported root. We weren't checking the strategy or any of its operands unless it was the original node. For any other node, we just rechecked the original node's strategy and operands.
The effect of this is that we don't do all of the transformations at once. Instead, when there were multiple possible nodes to transform we would only do them as each node was visited by the main DAG combine worklist.
The test shows a case where we widened an instruction without removing all of the uses of the vsext. The sext is shared by one node that shares another sext node with the root another node that doesn't share anything with the root.