Remove SDNPSideEffect from ARMcallseq_start and ARMcallseq_end (NFC) #153248
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
|
@llvm/pr-subscribers-backend-arm Author: AZero13 (AZero13) ChangesA call sequence does not have any unmodeled side effects in of itself. ADJCALLSTACKUP and ADJCALLSTACKDOWN do, however, so the attribute should be there. Finally, they are given empty scheduling info so the scheduler doesn't have to deal with it. Finally, make ADJCALLSTACKUP and ADJCALLSTACKDOWN codegen only. Full diff: https://github.com/llvm/llvm-project/pull/153248.diff 2 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td
index 934ec52c6f1e4..6907dd15524d4 100644
--- a/llvm/lib/Target/ARM/ARMInstrInfo.td
+++ b/llvm/lib/Target/ARM/ARMInstrInfo.td
@@ -164,10 +164,9 @@ def ARMWrapperPIC : SDNode<"ARMISD::WrapperPIC", SDTIntUnaryOp>;
def ARMWrapperJT : SDNode<"ARMISD::WrapperJT", SDTIntUnaryOp>;
def ARMcallseq_start : SDNode<"ISD::CALLSEQ_START", SDT_ARMCallSeqStart,
- [SDNPHasChain, SDNPSideEffect, SDNPOutGlue]>;
+ [SDNPHasChain, SDNPOutGlue]>;
def ARMcallseq_end : SDNode<"ISD::CALLSEQ_END", SDT_ARMCallSeqEnd,
- [SDNPHasChain, SDNPSideEffect,
- SDNPOptInGlue, SDNPOutGlue]>;
+ [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue]>;
def ARMcopystructbyval : SDNode<"ARMISD::COPY_STRUCT_BYVAL" ,
SDT_ARMStructByVal,
[SDNPHasChain, SDNPInGlue, SDNPOutGlue,
@@ -2203,18 +2202,16 @@ def JUMPTABLE_TBH :
PseudoInst<(outs), (ins cpinst_operand:$instid, cpinst_operand:$cpidx,
i32imm:$size), NoItinerary, []>;
-
-// FIXME: Marking these as hasSideEffects is necessary to prevent machine DCE
-// from removing one half of the matched pairs. That breaks PEI, which assumes
-// these will always be in pairs, and asserts if it finds otherwise. Better way?
-let Defs = [SP], Uses = [SP], hasSideEffects = 1 in {
+// We set Sched to empty list because we expect these instructions to simply get
+// removed in most cases.
+let Defs = [SP], Uses = [SP], hasSideEffects = 1, isCodeGenOnly = 1 in {
def ADJCALLSTACKUP :
PseudoInst<(outs), (ins i32imm:$amt1, i32imm:$amt2, pred:$p), NoItinerary,
- [(ARMcallseq_end timm:$amt1, timm:$amt2)]>;
+ [(ARMcallseq_end timm:$amt1, timm:$amt2)]>, Sched<[]>;
def ADJCALLSTACKDOWN :
PseudoInst<(outs), (ins i32imm:$amt, i32imm:$amt2, pred:$p), NoItinerary,
- [(ARMcallseq_start timm:$amt, timm:$amt2)]>;
+ [(ARMcallseq_start timm:$amt, timm:$amt2)]>, Sched<[]>;
}
def HINT : AI<(outs), (ins imm0_239:$imm), MiscFrm, NoItinerary,
diff --git a/llvm/lib/Target/ARM/ARMInstrThumb.td b/llvm/lib/Target/ARM/ARMInstrThumb.td
index e38cafdf55c46..b3dfbb9c4d281 100644
--- a/llvm/lib/Target/ARM/ARMInstrThumb.td
+++ b/llvm/lib/Target/ARM/ARMInstrThumb.td
@@ -297,19 +297,18 @@ def non_imm32 : PatLeaf<(i32 GPR), [{ return !isa<ConstantSDNode>(N); }]>;
// Miscellaneous Instructions.
//
-// FIXME: Marking these as hasSideEffects is necessary to prevent machine DCE
-// from removing one half of the matched pairs. That breaks PEI, which assumes
-// these will always be in pairs, and asserts if it finds otherwise. Better way?
-let Defs = [SP], Uses = [SP], hasSideEffects = 1 in {
+// We set Sched to empty list because we expect these instructions to simply get
+// removed in most cases.
+let Defs = [SP], Uses = [SP], hasSideEffects = 1, isCodeGenOnly = 1 in {
def tADJCALLSTACKUP :
PseudoInst<(outs), (ins i32imm:$amt1, i32imm:$amt2), NoItinerary,
[(ARMcallseq_end imm:$amt1, imm:$amt2)]>,
- Requires<[IsThumb, IsThumb1Only]>;
+ Requires<[IsThumb, IsThumb1Only]>, Sched<[]>;
def tADJCALLSTACKDOWN :
PseudoInst<(outs), (ins i32imm:$amt, i32imm:$amt2), NoItinerary,
[(ARMcallseq_start imm:$amt, imm:$amt2)]>,
- Requires<[IsThumb, IsThumb1Only]>;
+ Requires<[IsThumb, IsThumb1Only]>, Sched<[]>;
}
class T1SystemEncoding<bits<8> opc>
|
A call sequence does not have any unmodeled side effects in of itself. ADJCALLSTACKUP and ADJCALLSTACKDOWN do, however, so the attribute should be there. Finally, make ADJCALLSTACKUP and ADJCALLSTACKDOWN codegen only.
topperc
reviewed
Aug 14, 2025
ADJCALLSTACKUP and ADJCALLSTACKDOWN has the side effecs, not the sequence in itself.
davemgreen
reviewed
Aug 18, 2025
davemgreen
approved these changes
Aug 20, 2025
|
The tests seemed to do OK. Thanks |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/143/builds/10272 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.
A call sequence does not have any unmodeled side effects in of itself.
ADJCALLSTACKUP and ADJCALLSTACKDOWN do, however, so the attribute should be there.