From 55de8b20d24917e2ade631bf117030c3f8340d92 Mon Sep 17 00:00:00 2001 From: AZero13 Date: Tue, 12 Aug 2025 14:35:31 -0400 Subject: [PATCH 1/5] [ARM] Simplify ADJCALLSTACKUP and ADJCALLSTACKDOWN on ARM 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. --- llvm/lib/Target/ARM/ARMInstrInfo.td | 11 +++-------- llvm/lib/Target/ARM/ARMInstrThumb.td | 5 +---- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td index 934ec52c6f1e4..a3bf32446695e 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,11 +2202,7 @@ 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 { +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)]>; diff --git a/llvm/lib/Target/ARM/ARMInstrThumb.td b/llvm/lib/Target/ARM/ARMInstrThumb.td index e38cafdf55c46..3d23874e117aa 100644 --- a/llvm/lib/Target/ARM/ARMInstrThumb.td +++ b/llvm/lib/Target/ARM/ARMInstrThumb.td @@ -297,10 +297,7 @@ def non_imm32 : PatLeaf<(i32 GPR), [{ return !isa(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 { +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)]>, From e0d797a81b6862f1aea5c1b499a83c6de7390ffa Mon Sep 17 00:00:00 2001 From: AZero13 Date: Thu, 14 Aug 2025 14:05:21 -0400 Subject: [PATCH 2/5] Remove SDNPSideEffect from ARMcallseq_start and ARMcallseq_end ADJCALLSTACKUP and ADJCALLSTACKDOWN has the side effecs, not the sequence in itself. --- llvm/lib/Target/ARM/ARMInstrInfo.td | 2 +- llvm/lib/Target/ARM/ARMInstrThumb.td | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td index a3bf32446695e..a54672c460476 100644 --- a/llvm/lib/Target/ARM/ARMInstrInfo.td +++ b/llvm/lib/Target/ARM/ARMInstrInfo.td @@ -2202,7 +2202,7 @@ def JUMPTABLE_TBH : PseudoInst<(outs), (ins cpinst_operand:$instid, cpinst_operand:$cpidx, i32imm:$size), NoItinerary, []>; -let Defs = [SP], Uses = [SP], hasSideEffects = 1, isCodeGenOnly = 1 in { +let Defs = [SP], Uses = [SP], hasSideEffects = 1 in { def ADJCALLSTACKUP : PseudoInst<(outs), (ins i32imm:$amt1, i32imm:$amt2, pred:$p), NoItinerary, [(ARMcallseq_end timm:$amt1, timm:$amt2)]>; diff --git a/llvm/lib/Target/ARM/ARMInstrThumb.td b/llvm/lib/Target/ARM/ARMInstrThumb.td index 3d23874e117aa..483a432690c96 100644 --- a/llvm/lib/Target/ARM/ARMInstrThumb.td +++ b/llvm/lib/Target/ARM/ARMInstrThumb.td @@ -297,7 +297,7 @@ def non_imm32 : PatLeaf<(i32 GPR), [{ return !isa(N); }]>; // Miscellaneous Instructions. // -let Defs = [SP], Uses = [SP], hasSideEffects = 1, isCodeGenOnly = 1 in { +let Defs = [SP], Uses = [SP], hasSideEffects = 1 in { def tADJCALLSTACKUP : PseudoInst<(outs), (ins i32imm:$amt1, i32imm:$amt2), NoItinerary, [(ARMcallseq_end imm:$amt1, imm:$amt2)]>, From 99958a68ff8e2e460ef67001b38bcba963b6feaa Mon Sep 17 00:00:00 2001 From: AZero13 Date: Mon, 18 Aug 2025 11:44:25 -0400 Subject: [PATCH 3/5] Put comment back --- llvm/lib/Target/ARM/ARMInstrThumb.td | 3 +++ 1 file changed, 3 insertions(+) diff --git a/llvm/lib/Target/ARM/ARMInstrThumb.td b/llvm/lib/Target/ARM/ARMInstrThumb.td index 483a432690c96..e38cafdf55c46 100644 --- a/llvm/lib/Target/ARM/ARMInstrThumb.td +++ b/llvm/lib/Target/ARM/ARMInstrThumb.td @@ -297,6 +297,9 @@ def non_imm32 : PatLeaf<(i32 GPR), [{ return !isa(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 { def tADJCALLSTACKUP : PseudoInst<(outs), (ins i32imm:$amt1, i32imm:$amt2), NoItinerary, From 180a2e57c608bd205abd7ad59f4fc16e59742c95 Mon Sep 17 00:00:00 2001 From: AZero13 Date: Mon, 18 Aug 2025 11:46:25 -0400 Subject: [PATCH 4/5] Add comment back --- llvm/lib/Target/ARM/ARMInstrInfo.td | 3 +++ 1 file changed, 3 insertions(+) diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td index a54672c460476..231c26abf4143 100644 --- a/llvm/lib/Target/ARM/ARMInstrInfo.td +++ b/llvm/lib/Target/ARM/ARMInstrInfo.td @@ -2202,6 +2202,9 @@ 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 { def ADJCALLSTACKUP : PseudoInst<(outs), (ins i32imm:$amt1, i32imm:$amt2, pred:$p), NoItinerary, From daac1f8def04dd1491fb6a7032f611703de5b8a6 Mon Sep 17 00:00:00 2001 From: AZero13 Date: Tue, 19 Aug 2025 12:53:54 -0400 Subject: [PATCH 5/5] Update ARMInstrInfo.td --- llvm/lib/Target/ARM/ARMInstrInfo.td | 1 + 1 file changed, 1 insertion(+) diff --git a/llvm/lib/Target/ARM/ARMInstrInfo.td b/llvm/lib/Target/ARM/ARMInstrInfo.td index 231c26abf4143..4345f7a4a0ebc 100644 --- a/llvm/lib/Target/ARM/ARMInstrInfo.td +++ b/llvm/lib/Target/ARM/ARMInstrInfo.td @@ -2202,6 +2202,7 @@ 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?