[ARM][Thumb2] Mark BTI-clearing instructions as scheduling region boundaries #79173
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: Victor Campos (vhscampos) ChangesFollowing #68313 this patch extends the idea to M-profile PACBTI. The Machine Scheduler can reorder instructions within a scheduling region depending on the scheduling policy set. If a BTI-clearing instruction happens to partake in one such region, it might be moved around, therefore ending up where it shouldn't. The solution is to mark all BTI-clearing instructions as scheduling region boundaries. This essentially means that they must not be part of any scheduling region, and as consequence never get moved:
Note that PAC isn't BTI-clearing, but it's replaced by PACBTI late in the compilation pipeline. As far as I know, currently it isn't possible to organically obtain code that's susceptible to the bug:
Nevertheless, one can reasonably argue that we should prevent the bug in spite of the compiler not being able to produce the required conditions for it. If things change, the compiler will be robust against this issue. The tests written for this are contrived: bogus MIR instructions have been added adjacent to the BTI-clearing instructions in order to have them inside non-trivial scheduling regions. Full diff: https://github.com/llvm/llvm-project/pull/79173.diff 4 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index 4bf65be6f10262e..5ae81698583df59 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -2088,7 +2088,7 @@ bool ARMBaseInstrInfo::isSchedulingBoundary(const MachineInstr &MI,
if (!MI.isCall() && MI.definesRegister(ARM::SP))
return true;
- return false;
+ return TargetInstrInfo::isSchedulingBoundary(MI, MBB, MF);
}
bool ARMBaseInstrInfo::
diff --git a/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp b/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
index 083f25f49dec459..8d898cf5fc18f3a 100644
--- a/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
+++ b/llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
@@ -286,6 +286,26 @@ MachineInstr *Thumb2InstrInfo::commuteInstructionImpl(MachineInstr &MI,
return ARMBaseInstrInfo::commuteInstructionImpl(MI, NewMI, OpIdx1, OpIdx2);
}
+bool Thumb2InstrInfo::isSchedulingBoundary(const MachineInstr &MI,
+ const MachineBasicBlock *MBB,
+ const MachineFunction &MF) const {
+ // BTI clearing instructions shall not take part in scheduling regions as
+ // they must stay in their intended place. Although PAC isn't BTI clearing,
+ // it can be transformed into PACBTI after the pre-RA Machine Scheduling
+ // has taken place, so its movement must also be restricted.
+ switch (MI.getOpcode()) {
+ case ARM::t2BTI:
+ case ARM::t2PAC:
+ case ARM::t2PACBTI:
+ case ARM::t2CALL_BTI:
+ case ARM::t2SG:
+ return true;
+ default:
+ break;
+ }
+ return ARMBaseInstrInfo::isSchedulingBoundary(MI, MBB, MF);
+}
+
void llvm::emitT2RegPlusImmediate(MachineBasicBlock &MBB,
MachineBasicBlock::iterator &MBBI,
const DebugLoc &dl, Register DestReg,
diff --git a/llvm/lib/Target/ARM/Thumb2InstrInfo.h b/llvm/lib/Target/ARM/Thumb2InstrInfo.h
index 4bb412f09dcbeb3..8915da8c5bf3c8f 100644
--- a/llvm/lib/Target/ARM/Thumb2InstrInfo.h
+++ b/llvm/lib/Target/ARM/Thumb2InstrInfo.h
@@ -68,6 +68,10 @@ class Thumb2InstrInfo : public ARMBaseInstrInfo {
unsigned OpIdx1,
unsigned OpIdx2) const override;
+ bool isSchedulingBoundary(const MachineInstr &MI,
+ const MachineBasicBlock *MBB,
+ const MachineFunction &MF) const override;
+
private:
void expandLoadStackGuard(MachineBasicBlock::iterator MI) const override;
};
diff --git a/llvm/test/CodeGen/ARM/misched-branch-targets.mir b/llvm/test/CodeGen/ARM/misched-branch-targets.mir
new file mode 100644
index 000000000000000..6f09644b227cec3
--- /dev/null
+++ b/llvm/test/CodeGen/ARM/misched-branch-targets.mir
@@ -0,0 +1,181 @@
+# RUN: llc -o - -run-pass=machine-scheduler -misched=shuffle %s | FileCheck %s
+# RUN: llc -o - -run-pass=postmisched %s | FileCheck %s
+
+--- |
+ target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
+ target triple = "thumbv8.1m.main-arm-none-eabi"
+
+ define dso_local i32 @foo_bti(i32 noundef %a) local_unnamed_addr #7 {
+ entry:
+ %add = add nsw i32 %a, 1
+ ret i32 %add
+ }
+
+ define dso_local i32 @foo_pacbti(i32 noundef %a) local_unnamed_addr #7 {
+ entry:
+ %add = add nsw i32 %a, 1
+ ret i32 %add
+ }
+
+ define dso_local noundef i32 @foo_setjmp() local_unnamed_addr #0 {
+ entry:
+ %buf = alloca [20 x i64], align 8
+ call void @llvm.lifetime.start.p0(i64 160, ptr nonnull %buf) #4
+ %call = call i32 @setjmp(ptr noundef nonnull %buf) #5
+ %tobool.not = icmp eq i32 %call, 0
+ br i1 %tobool.not, label %if.else, label %if.then
+
+ if.then: ; preds = %entry
+ call void @longjmp(ptr noundef nonnull %buf, i32 noundef 1) #6
+ unreachable
+
+ if.else: ; preds = %entry
+ call void @llvm.lifetime.end.p0(i64 160, ptr nonnull %buf) #4
+ ret i32 0
+ }
+
+ declare void @llvm.lifetime.start.p0(i64 immarg, ptr nocapture) #1
+ declare dso_local i32 @setjmp(ptr noundef) local_unnamed_addr #2
+ declare dso_local void @longjmp(ptr noundef, i32 noundef) local_unnamed_addr #3
+ declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture) #1
+
+ attributes #0 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-m55" "target-features"="+armv8.1-m.main,+dsp,+fp-armv8d16,+fp-armv8d16sp,+fp16,+fp64,+fullfp16,+hwdiv,+lob,+mve,+mve.fp,+ras,+strict-align,+thumb-mode,+vfp2,+vfp2sp,+vfp3d16,+vfp3d16sp,+vfp4d16,+vfp4d16sp,-aes,-bf16,-cdecp0,-cdecp1,-cdecp2,-cdecp3,-cdecp4,-cdecp5,-cdecp6,-cdecp7,-crc,-crypto,-d32,-dotprod,-fp-armv8,-fp-armv8sp,-fp16fml,-hwdiv-arm,-i8mm,-neon,-pacbti,-sb,-sha2,-vfp3,-vfp3sp,-vfp4,-vfp4sp" }
+ attributes #1 = { nocallback nofree nosync nounwind willreturn memory(argmem: readwrite) }
+ attributes #2 = { nounwind returns_twice "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-m55" "target-features"="+armv8.1-m.main,+dsp,+fp-armv8d16,+fp-armv8d16sp,+fp16,+fp64,+fullfp16,+hwdiv,+lob,+mve,+mve.fp,+ras,+strict-align,+thumb-mode,+vfp2,+vfp2sp,+vfp3d16,+vfp3d16sp,+vfp4d16,+vfp4d16sp,-aes,-bf16,-cdecp0,-cdecp1,-cdecp2,-cdecp3,-cdecp4,-cdecp5,-cdecp6,-cdecp7,-crc,-crypto,-d32,-dotprod,-fp-armv8,-fp-armv8sp,-fp16fml,-hwdiv-arm,-i8mm,-neon,-pacbti,-sb,-sha2,-vfp3,-vfp3sp,-vfp4,-vfp4sp" }
+ attributes #3 = { noreturn nounwind "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-m55" "target-features"="+armv8.1-m.main,+dsp,+fp-armv8d16,+fp-armv8d16sp,+fp16,+fp64,+fullfp16,+hwdiv,+lob,+mve,+mve.fp,+ras,+strict-align,+thumb-mode,+vfp2,+vfp2sp,+vfp3d16,+vfp3d16sp,+vfp4d16,+vfp4d16sp,-aes,-bf16,-cdecp0,-cdecp1,-cdecp2,-cdecp3,-cdecp4,-cdecp5,-cdecp6,-cdecp7,-crc,-crypto,-d32,-dotprod,-fp-armv8,-fp-armv8sp,-fp16fml,-hwdiv-arm,-i8mm,-neon,-pacbti,-sb,-sha2,-vfp3,-vfp3sp,-vfp4,-vfp4sp" }
+ attributes #4 = { nounwind }
+ attributes #5 = { nounwind returns_twice }
+ attributes #6 = { noreturn nounwind }
+ attributes #7 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="cortex-m55" "target-features"="+armv8.1-m.main,+dsp,+fp-armv8d16,+fp-armv8d16sp,+fp16,+fp64,+fullfp16,+hwdiv,+lob,+mve,+mve.fp,+ras,+strict-align,+thumb-mode,+vfp2,+vfp2sp,+vfp3d16,+vfp3d16sp,+vfp4d16,+vfp4d16sp,-aes,-bf16,-cdecp0,-cdecp1,-cdecp2,-cdecp3,-cdecp4,-cdecp5,-cdecp6,-cdecp7,-crc,-crypto,-d32,-dotprod,-fp-armv8,-fp-armv8sp,-fp16fml,-hwdiv-arm,-i8mm,-neon,-pacbti,-sb,-sha2,-vfp3,-vfp3sp,-vfp4,-vfp4sp" }
+
+...
+---
+name: foo_bti
+alignment: 4
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+ - { reg: '$r0' }
+frameInfo:
+ maxAlignment: 1
+ maxCallFrameSize: 0
+machineFunctionInfo:
+ isLRSpilled: false
+body: |
+ bb.0.entry:
+ liveins: $r0
+
+ t2BTI
+ renamable $r0, dead $cpsr = nsw tADDi8 killed renamable $r0, 1, 14 /* CC::al */, $noreg
+ tBX_RET 14 /* CC::al */, $noreg, implicit killed $r0
+
+...
+
+# CHECK-LABEL: name: foo_bti
+# CHECK: body:
+# CHECK-NEXT: bb.0.entry:
+# CHECK-NEXT: liveins: $r0
+# CHECK-NEXT: {{^ +$}}
+# CHECK-NEXT: t2BTI
+
+---
+name: foo_pacbti
+alignment: 4
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+ - { reg: '$r0' }
+frameInfo:
+ stackSize: 12
+ offsetAdjustment: -4
+ maxAlignment: 4
+ maxCallFrameSize: 0
+stack:
+ - { id: 0, type: spill-slot, offset: -4, size: 4, alignment: 4, callee-saved-register: '$lr' }
+ - { id: 1, type: spill-slot, offset: -8, size: 4, alignment: 4, callee-saved-register: '$r7' }
+ - { id: 2, type: spill-slot, offset: -12, size: 4, alignment: 4, callee-saved-register: '$r12' }
+machineFunctionInfo:
+ isLRSpilled: true
+body: |
+ bb.0.entry:
+ liveins: $r0, $lr, $r12
+
+ frame-setup t2PAC implicit-def $r12, implicit $lr, implicit $sp
+ renamable $r2 = nsw t2ADDri $r0, 3, 14 /* CC::al */, $noreg, $noreg
+ $sp = frame-setup t2STMDB_UPD $sp, 14 /* CC::al */, $noreg, killed $r7, killed $lr
+ frame-setup CFI_INSTRUCTION def_cfa_offset 8
+ frame-setup CFI_INSTRUCTION offset $lr, -4
+ frame-setup CFI_INSTRUCTION offset $r7, -8
+ $r7 = frame-setup tMOVr killed $sp, 14 /* CC::al */, $noreg
+ frame-setup CFI_INSTRUCTION def_cfa_register $r7
+ early-clobber $sp = frame-setup t2STR_PRE killed $r12, $sp, -4, 14 /* CC::al */, $noreg
+ frame-setup CFI_INSTRUCTION offset $ra_auth_code, -12
+ renamable $r0 = nsw t2ADDri killed renamable $r0, 1, 14 /* CC::al */, $noreg, $noreg
+ $r12, $sp = frame-destroy t2LDR_POST $sp, 4, 14 /* CC::al */, $noreg
+ $sp = frame-destroy t2LDMIA_UPD $sp, 14 /* CC::al */, $noreg, def $r7, def $lr
+ t2AUT implicit $r12, implicit $lr, implicit $sp
+ tBX_RET 14 /* CC::al */, $noreg, implicit $r0
+
+...
+
+# CHECK-LABEL: name: foo_pacbti
+# CHECK: body:
+# CHECK-NEXT: bb.0.entry:
+# CHECK-NEXT: liveins: $r0, $lr, $r12
+# CHECK-NEXT: {{^ +$}}
+# CHECK-NEXT: frame-setup t2PAC implicit-def $r12, implicit $lr, implicit $sp
+
+---
+name: foo_setjmp
+alignment: 4
+exposesReturnsTwice: true
+tracksRegLiveness: true
+tracksDebugUserValues: true
+frameInfo:
+ stackSize: 168
+ offsetAdjustment: -160
+ maxAlignment: 8
+ adjustsStack: true
+ hasCalls: true
+ maxCallFrameSize: 0
+ localFrameSize: 160
+stack:
+ - { id: 0, name: buf, offset: -168, size: 160, alignment: 8, local-offset: -160 }
+ - { id: 1, type: spill-slot, offset: -4, size: 4, alignment: 4, callee-saved-register: '$lr',
+ callee-saved-restored: false }
+ - { id: 2, type: spill-slot, offset: -8, size: 4, alignment: 4, callee-saved-register: '$r7' }
+machineFunctionInfo:
+ isLRSpilled: true
+body: |
+ bb.0.entry:
+ successors: %bb.1
+ liveins: $lr
+
+ frame-setup tPUSH 14 /* CC::al */, $noreg, $r7, killed $lr, implicit-def $sp, implicit $sp
+ frame-setup CFI_INSTRUCTION def_cfa_offset 8
+ frame-setup CFI_INSTRUCTION offset $lr, -4
+ frame-setup CFI_INSTRUCTION offset $r7, -8
+ $r7 = frame-setup tMOVr $sp, 14 /* CC::al */, $noreg
+ frame-setup CFI_INSTRUCTION def_cfa_register $r7
+ $sp = frame-setup tSUBspi $sp, 40, 14 /* CC::al */, $noreg
+ renamable $r0 = tMOVr $sp, 14 /* CC::al */, $noreg
+ tBL 14 /* CC::al */, $noreg, @setjmp, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit killed $r0, implicit-def $sp, implicit-def $r0
+ t2BTI
+ renamable $r2 = nsw t2ADDri $r0, 3, 14 /* CC::al */, $noreg, $noreg
+ tCMPi8 killed renamable $r0, 0, 14 /* CC::al */, $noreg, implicit-def $cpsr
+ t2IT 0, 2, implicit-def $itstate
+ renamable $r0 = tMOVi8 $noreg, 0, 0 /* CC::eq */, $cpsr, implicit $itstate
+ $sp = frame-destroy tADDspi $sp, 40, 0 /* CC::eq */, $cpsr, implicit $itstate
+ frame-destroy tPOP_RET 0 /* CC::eq */, killed $cpsr, def $r7, def $pc, implicit killed $r0, implicit $sp, implicit killed $itstate
+
+ bb.1.if.then:
+ renamable $r0 = tMOVr $sp, 14 /* CC::al */, $noreg
+ renamable $r1, dead $cpsr = tMOVi8 1, 14 /* CC::al */, $noreg
+ tBL 14 /* CC::al */, $noreg, @longjmp, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit killed $r0, implicit killed $r1, implicit-def $sp
+
+...
+
+# CHECK-LABEL: name: foo_setjmp
+# CHECK: body:
+# CHECK: tBL 14 /* CC::al */, $noreg, @setjmp, csr_aapcs, implicit-def dead $lr, implicit $sp, implicit killed $r0, implicit-def $sp, implicit-def $r0
+# CHECK-NEXT: t2BTI
|
atrosinenko
reviewed
Jan 24, 2024
Following llvm#68313 this patch extends the idea to M-profile PACBTI. The Machine Scheduler can reorder instructions within a scheduling region depending on the scheduling policy set. If a BTI-clearing instruction happens to partake in one such region, it might be moved around, therefore ending up where it shouldn't. The solution is to mark all BTI-clearing instructions as scheduling region boundaries. This essentially means that they must not be part of any scheduling region, and as consequence never get moved: - PAC - PACBTI - BTI - SG - CALL_BTI (pseudo-instruction for setjmp + bti) Note that PAC isn't BTI-clearing, but it's replaced by PACBTI late in the compilation pipeline. As far as I know, currently it isn't possible to organically obtain code that's susceptible to the bug: - Instructions that write to SP are region boundaries. PAC seems to always be followed by the pushing of r12 to the stack, so essentially PAC is always by itself in a scheduling region. - CALL_BTI is expanded into a machine instruction bundle. Bundles are unpacked only after the last machine scheduler run. Thus setjmp and BTI can be separated only if someone deliberately runs the scheduler once more. - The BTI insertion pass is run late in the pipeline, only after the last machine scheduling has run. So once again it can be reordered only if someone deliberately runs the scheduler again. Nevertheless, one can reasonably argue that we should prevent the bug in spite of the compiler not being able to produce the required conditions for it. If things change, the compiler will be robust against this issue. The tests written for this are contrived: bogus MIR instructions have been added adjacent to the BTI-clearing instructions in order to have them inside non-trivial scheduling regions.
atrosinenko
reviewed
Jan 31, 2024
There was a problem hiding this comment.
A few thoughts/suggestions on the tests (just an opinion):
- the test can probably be further simplified by dropping unrelated machine instructions or replacing them with obvious NOPs for readability
- AFAIK in their inline LLVM IR modules, some mir-based tests use dummy function definitions as placeholders - not corresponding to the actual machine instructions (probably, some properties of the original functions have to be kept)
- it may be worth adding simple test cases for the other BTI-clearing instructions
Addressed comments.
I've removed one or two extra instructions. I reckon that what's left is important because it's intrinsic to the prolog/epilog codegen of PAC and BTI, although some of it is not necessary to trigger the bug.
Good point. Now all the IR definitions are as dummy as possible.
Added one test for SG and another for PACBTI. I removed t2CALL_BTI from the list of scheduling boundaries because it's a pseudo-instruction that is transformed into BL + BTI. The pseudo-inst itself can be moved around with no issues. After the expansion, the restrictions must then be enforced. |
|
@atrosinenko Can you please have a look at the latest patch? Thanks |
atrosinenko
approved these changes
Mar 30, 2024
|
Thanks @atrosinenko . I've resolved the attribute list duplication. |
|
FYI, we are seeing test failures on It complains |
|
We understand the issue now. The The failing bot was building the LLVM in release mode, therefore, the flag doesn't exist. |
|
Patch reverted while I fix the issue. Thanks for reporting @zeroomega |
artagnon
reviewed
Apr 5, 2024
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.
Following #68313 this patch extends the idea to M-profile PACBTI.
The Machine Scheduler can reorder instructions within a scheduling region depending on the scheduling policy set. If a BTI-clearing instruction happens to partake in one such region, it might be moved around, therefore ending up where it shouldn't.
The solution is to mark all BTI-clearing instructions as scheduling region boundaries. This essentially means that they must not be part of any scheduling region, and as consequence never get moved:
Note that PAC isn't BTI-clearing, but it's replaced by PACBTI late in the compilation pipeline.
As far as I know, currently it isn't possible to organically obtain code that's susceptible to the bug:
Nevertheless, one can reasonably argue that we should prevent the bug in spite of the compiler not being able to produce the required conditions for it. If things change, the compiler will be robust against this issue.
The tests written for this are contrived: bogus MIR instructions have been added adjacent to the BTI-clearing instructions in order to have them inside non-trivial scheduling regions.