[SelectionDAG][PowerPC] Remove setTruncatingStore from StoreSDNode. #137667
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
Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.
|
@llvm/pr-subscribers-backend-powerpc @llvm/pr-subscribers-llvm-selectiondag Author: Craig Topper (topperc) ChangesMutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist. Full diff: https://github.com/llvm/llvm-project/pull/137667.diff 2 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
index b279ca90be9e8..c89eba1b30ed1 100644
--- a/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
+++ b/llvm/include/llvm/CodeGen/SelectionDAGNodes.h
@@ -2533,9 +2533,6 @@ class StoreSDNode : public LSBaseSDNode {
/// For integers this is the same as doing a TRUNCATE and storing the result.
/// For floats, it is the same as doing an FP_ROUND and storing the result.
bool isTruncatingStore() const { return StoreSDNodeBits.IsTruncating; }
- void setTruncatingStore(bool Truncating) {
- StoreSDNodeBits.IsTruncating = Truncating;
- }
const SDValue &getValue() const { return getOperand(1); }
const SDValue &getBasePtr() const { return getOperand(2); }
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 0800ed5dfce2c..a4d4294fe9efc 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -16562,13 +16562,16 @@ SDValue PPCTargetLowering::PerformDAGCombine(SDNode *N,
MemVT.getSizeInBits());
SDValue Const64 = DAG.getConstant(Val64, dl, MVT::i64);
- // DAG.getTruncStore() can't be used here because it doesn't accept
- // the general (base + offset) addressing mode.
- // So we use UpdateNodeOperands and setTruncatingStore instead.
- DAG.UpdateNodeOperands(N, N->getOperand(0), Const64, N->getOperand(2),
- N->getOperand(3));
- cast<StoreSDNode>(N)->setTruncatingStore(true);
- return SDValue(N, 0);
+ auto *ST = cast<StoreSDNode>(N);
+ SDValue NewST = DAG.getStore(ST->getChain(), dl, Const64,
+ ST->getBasePtr(), ST->getOffset(), MemVT,
+ ST->getMemOperand(), ST->getAddressingMode(),
+ /*IsTruncating=*/true);
+ // Note we use CombineTo here to prevent DAGCombine from visiting the
+ // new store which will change the constant by removing non-demanded bits.
+ return ST->isUnindexed()
+ ? DCI.CombineTo(N, NewST, /*AddTo=*/false)
+ : DCI.CombineTo(N, NewST, NewST.getValue(1), /*AddTo=*/false);
}
// For little endian, VSX stores require generating xxswapd/lxvd2x.
|
Co-authored-by: Sergei Barannikov <[email protected]>
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Apr 28, 2025
…isters. If there's an implicit-def of a super register, the propagation must preserve this implicit-def. Knowing how and when to do this may require target specific knowledge so just disable it for now. Prior to e17f07c, we checked that the copy had explicit 2 operands when that was removed we started allowing implicit operands through. This patch adds a check for implicit operands, but still allows extra explicit operands which was the goal of e17f07c. Fixes llvm#137667.
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Apr 28, 2025
…isters. If there's an implicit-def of a super register, the propagation must preserve this implicit-def. Knowing how and when to do this may require target specific knowledge so just disable it for now. Prior to e17f07c, we checked that the copy had explicit 2 operands when that was removed we started allowing implicit operands through. This patch adds a check for implicit operands, but still allows extra explicit operands which was the goal of e17f07c. Fixes llvm#137667.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/8591 Here is the relevant piece of the build log for the reference |
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…lvm#137667) Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…lvm#137667) Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
…lvm#137667) Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.
GeorgeARM
pushed a commit
to GeorgeARM/llvm-project
that referenced
this pull request
May 7, 2025
…lvm#137667) Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
…lvm#137667) Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC. Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.
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.
Mutating a node after it has been created isn't a good idea. After e17f07c, we have a version of setStore that can create a truncating indexed store. Use that instead of MorphNodeTo+setTruncatingStore in PowerPC.
Unfortunately, if we return the newly created node, DAGCombiner will visit the node and change the constant. To prevent this, we use DCI.CombineTo and avoid adding the new node to the worklist.