[ARM] Add a method to clear registers #69659
Conversation
This allows us to clear registers in target-independent code. The first use will be for clearing the stack protector from registers before returning.
|
@llvm/pr-subscribers-backend-arm Author: Bill Wendling (bwendling) ChangesThis allows us to clear registers in target-independent code. The first Full diff: https://github.com/llvm/llvm-project/pull/69659.diff 2 Files Affected:
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
index 1ffdde0360cf623..a07504aaedbc4da 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
@@ -6735,6 +6735,16 @@ MachineBasicBlock::iterator ARMBaseInstrInfo::insertOutlinedCall(
return CallPt;
}
+void ARMBaseInstrInfo::buildClearRegister(Register DstReg,
+ MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator Iter,
+ DebugLoc &DL,
+ bool AllowSideEffects) const {
+ unsigned Opc = Subtarget.isThumb2() ? ARM::t2MOVi32imm : ARM::MOVi32imm;
+ BuildMI(MBB, Iter, DL, get(Opc), DstReg)
+ .addImm(0);
+}
+
bool ARMBaseInstrInfo::shouldOutlineFromFunctionByDefault(
MachineFunction &MF) const {
return Subtarget.isMClass() && MF.getFunction().hasMinSize();
diff --git a/llvm/lib/Target/ARM/ARMBaseInstrInfo.h b/llvm/lib/Target/ARM/ARMBaseInstrInfo.h
index 5efcc1a0d9fc073..44b3e528c0c242e 100644
--- a/llvm/lib/Target/ARM/ARMBaseInstrInfo.h
+++ b/llvm/lib/Target/ARM/ARMBaseInstrInfo.h
@@ -363,6 +363,11 @@ class ARMBaseInstrInfo : public ARMGenInstrInfo {
MachineBasicBlock::iterator &It, MachineFunction &MF,
outliner::Candidate &C) const override;
+ void buildClearRegister(Register Reg, MachineBasicBlock &MBB,
+ MachineBasicBlock::iterator Iter,
+ DebugLoc &DL,
+ bool AllowSideEffects = true) const override;
+
/// Enable outlining by default at -Oz.
bool shouldOutlineFromFunctionByDefault(MachineFunction &MF) const override;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| MachineBasicBlock::iterator Iter, | ||
| DebugLoc &DL, | ||
| bool AllowSideEffects) const { | ||
| unsigned Opc = Subtarget.isThumb2() ? ARM::t2MOVi32imm : ARM::MOVi32imm; |
There was a problem hiding this comment.
Should floating-point registers also be covered by this method?
There was a problem hiding this comment.
Also, do we care about Thumb1?
There was a problem hiding this comment.
Should floating-point registers also be covered by this method?
Yes. I forgot about them. :-(
Also, do we care about Thumb1?
I can change this to isThumb(), but I want to avoid using tMOVi32imm, because it modifies CPSR (I want to avoid side effects even if AllowSideEffects is true).
There was a problem hiding this comment.
(I want to avoid side effects even if AllowSideEffects is true).
huh? Why?
I suspect you'll need to check:
if arm:
...
elif thumb2:
...
else: # is thumb1
...
There was a problem hiding this comment.
Avoiding side effects should always be desirable / safe. The only reason to allow them is if an alternative instruction (e.g. xor on x86) is smaller or more efficient or whatever. In the case of ARM / AArch64, the size of the instruction doesn't matter of course. However, ARM does produce code like this when exiting the function with stack protectors:
cmp r1, r0
addeq sp, sp, #8
addeq sp, sp, #1024
popeq {r11, pc}
So shoving a side-effect after the cmp instruction won't be good. Yes, I will have AllowSideEffects == false in that case, but I don't see why we would allow side-effects at all unless it's clearly a win.
After looking at the documentation, it does appear that the Thumb1 version of MOV Rn, #imm won't modify the condition flags, unless we specify it to. I'll make that change.
| MachineBasicBlock::iterator Iter, | ||
| DebugLoc &DL, | ||
| bool AllowSideEffects) const { | ||
| unsigned Opc = Subtarget.isThumb2() ? ARM::t2MOVi32imm : ARM::MOVi32imm; |
There was a problem hiding this comment.
(I want to avoid side effects even if AllowSideEffects is true).
huh? Why?
I suspect you'll need to check:
if arm:
...
elif thumb2:
...
else: # is thumb1
...
|
If you'd like, I could wait for the full "clear stack protector register" patch and submit this with it... |
|
@nickdesaulniers Thanks for the approval, but I've decided that I'll submit this with the other code I'm writing. That way I'll have testcases. :-) |
This allows us to clear registers in target-independent code. The first
use will be for clearing the stack protector from registers before
returning.