[M68k] always use movem for register spills #106715
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-backend-m68k Author: Janis Heims (TechnoElf) ChangesFixes #106206 and #106209. Full diff: https://github.com/llvm/llvm-project/pull/106715.diff 4 Files Affected:
diff --git a/llvm/lib/Target/M68k/M68kExpandPseudo.cpp b/llvm/lib/Target/M68k/M68kExpandPseudo.cpp
index c7fdd7d7c35023..1ba265a60c3d60 100644
--- a/llvm/lib/Target/M68k/M68kExpandPseudo.cpp
+++ b/llvm/lib/Target/M68k/M68kExpandPseudo.cpp
@@ -193,31 +193,23 @@ bool M68kExpandPseudo::ExpandMI(MachineBasicBlock &MBB,
case M68k::MOV8dc:
return TII->ExpandCCR(MIB, /*IsToCCR=*/false);
- case M68k::MOVM8jm_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32jm), /*IsRM=*/false);
case M68k::MOVM16jm_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32jm), /*IsRM=*/false);
+ return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM16jm), /*IsRM=*/false);
case M68k::MOVM32jm_P:
return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32jm), /*IsRM=*/false);
- case M68k::MOVM8pm_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32pm), /*IsRM=*/false);
case M68k::MOVM16pm_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32pm), /*IsRM=*/false);
+ return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM16pm), /*IsRM=*/false);
case M68k::MOVM32pm_P:
return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32pm), /*IsRM=*/false);
- case M68k::MOVM8mj_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32mj), /*IsRM=*/true);
case M68k::MOVM16mj_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32mj), /*IsRM=*/true);
+ return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM16mj), /*IsRM=*/true);
case M68k::MOVM32mj_P:
return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32mj), /*IsRM=*/true);
- case M68k::MOVM8mp_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32mp), /*IsRM=*/true);
case M68k::MOVM16mp_P:
- return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32mp), /*IsRM=*/true);
+ return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM16mp), /*IsRM=*/true);
case M68k::MOVM32mp_P:
return TII->ExpandMOVEM(MIB, TII->get(M68k::MOVM32mp), /*IsRM=*/true);
diff --git a/llvm/lib/Target/M68k/M68kInstrData.td b/llvm/lib/Target/M68k/M68kInstrData.td
index dc777a933e2786..48aa8aeb667db6 100644
--- a/llvm/lib/Target/M68k/M68kInstrData.td
+++ b/llvm/lib/Target/M68k/M68kInstrData.td
@@ -337,20 +337,16 @@ class MxMOVEM_RM_Pseudo<MxType TYPE, MxOperand MEMOp>
: MxPseudo<(outs TYPE.ROp:$dst), (ins MEMOp:$src)>;
// Mem <- Reg
-def MOVM8jm_P : MxMOVEM_MR_Pseudo<MxType8d, MxType8.JOp>;
def MOVM16jm_P : MxMOVEM_MR_Pseudo<MxType16r, MxType16.JOp>;
def MOVM32jm_P : MxMOVEM_MR_Pseudo<MxType32r, MxType32.JOp>;
-def MOVM8pm_P : MxMOVEM_MR_Pseudo<MxType8d, MxType8.POp>;
def MOVM16pm_P : MxMOVEM_MR_Pseudo<MxType16r, MxType16.POp>;
def MOVM32pm_P : MxMOVEM_MR_Pseudo<MxType32r, MxType32.POp>;
// Reg <- Mem
-def MOVM8mj_P : MxMOVEM_RM_Pseudo<MxType8d, MxType8.JOp>;
def MOVM16mj_P : MxMOVEM_RM_Pseudo<MxType16r, MxType16.JOp>;
def MOVM32mj_P : MxMOVEM_RM_Pseudo<MxType32r, MxType32.JOp>;
-def MOVM8mp_P : MxMOVEM_RM_Pseudo<MxType8d, MxType8.POp>;
def MOVM16mp_P : MxMOVEM_RM_Pseudo<MxType16r, MxType16.POp>;
def MOVM32mp_P : MxMOVEM_RM_Pseudo<MxType32r, MxType32.POp>;
diff --git a/llvm/lib/Target/M68k/M68kInstrInfo.cpp b/llvm/lib/Target/M68k/M68kInstrInfo.cpp
index 23c5c76a47479b..d9fe7193a79377 100644
--- a/llvm/lib/Target/M68k/M68kInstrInfo.cpp
+++ b/llvm/lib/Target/M68k/M68kInstrInfo.cpp
@@ -542,7 +542,6 @@ bool M68kInstrInfo::ExpandCCR(MachineInstrBuilder &MIB, bool IsToCCR) const {
bool M68kInstrInfo::ExpandMOVEM(MachineInstrBuilder &MIB,
const MCInstrDesc &Desc, bool IsRM) const {
int Reg = 0, Offset = 0, Base = 0;
- auto XR32 = RI.getRegClass(M68k::XR32RegClassID);
auto DL = MIB->getDebugLoc();
auto MI = MIB.getInstr();
auto &MBB = *MIB->getParent();
@@ -557,13 +556,6 @@ bool M68kInstrInfo::ExpandMOVEM(MachineInstrBuilder &MIB,
Reg = MIB->getOperand(2).getReg();
}
- // If the register is not in XR32 then it is smaller than 32 bit, we
- // implicitly promote it to 32
- if (!XR32->contains(Reg)) {
- Reg = RI.getMatchingMegaReg(Reg, XR32);
- assert(Reg && "Has not meaningful MEGA register");
- }
-
unsigned Mask = 1 << RI.getSpillRegisterOrder(Reg);
if (IsRM) {
BuildMI(MBB, MI, DL, Desc)
@@ -738,14 +730,12 @@ unsigned getLoadStoreRegOpcode(unsigned Reg, const TargetRegisterClass *RC,
default:
llvm_unreachable("Unknown spill size");
case 8:
- if (M68k::DR8RegClass.hasSubClassEq(RC))
- return load ? M68k::MOV8dp : M68k::MOV8pd;
- if (M68k::CCRCRegClass.hasSubClassEq(RC))
- return load ? M68k::MOV16cp : M68k::MOV16pc;
-
- llvm_unreachable("Unknown 1-byte regclass");
+ assert(M68k::CCRCRegClass.hasSubClassEq(RC) && "Unknown 1-byte regclass");
+ return load ? M68k::MOV16cp : M68k::MOV16pc;
case 16:
- assert(M68k::XR16RegClass.hasSubClassEq(RC) && "Unknown 2-byte regclass");
+ assert(M68k::XR16RegClass.hasSubClassEq(RC) ||
+ M68k::DR8RegClass.hasSubClassEq(RC) &&
+ "Unknown 2-byte regclass");
return load ? M68k::MOVM16mp_P : M68k::MOVM16pm_P;
case 32:
assert(M68k::XR32RegClass.hasSubClassEq(RC) && "Unknown 4-byte regclass");
diff --git a/llvm/lib/Target/M68k/M68kRegisterInfo.td b/llvm/lib/Target/M68k/M68kRegisterInfo.td
index 45b492eba4ec07..96741344f0330f 100644
--- a/llvm/lib/Target/M68k/M68kRegisterInfo.td
+++ b/llvm/lib/Target/M68k/M68kRegisterInfo.td
@@ -99,7 +99,9 @@ class MxRegClass<list<ValueType> regTypes, int alignment, dag regList>
: RegisterClass<"M68k", regTypes, alignment, regList>;
// Data Registers
+let Size = 16 in
def DR8 : MxRegClass<[i8], 16, (sequence "BD%u", 0, 7)>;
+
def DR16 : MxRegClass<[i16], 16, (sequence "WD%u", 0, 7)>;
def DR32 : MxRegClass<[i32], 32, (sequence "D%u", 0, 7)>;
|
TechnoElf
force-pushed
the
m68k-movem-spill
branch
from
dc61bca to
b3f427b
Compare
|
ping @mshockwave @0x59616e |
|
I apologize for the long wait. I have some free time recently and I will give some feedback ASAP. |
|
Thanks for this fix! I have one question: How can we ensure that our test targets the exact issue we're trying to address? To be more specific, we are addressing two issues: spill instruction & spill size, and I don't know how these two issues are being targeted with the llvm/test/CodeGen/M68k/PR57660.ll. It would be great if you could provide more details. Thanks! |
|
To be honest, I have no idea how to test for the correct spill size. I guess it may be possible to force a spill of a specific size by assigning more variables of that size than there are registers, but I'm not sure if that would guarantee this behavior. Only checking the generated instructions also wouldn't ensure that enough space for the variable actually gets allocated on the stack. I didn't add any tests specifically for this issue thus far, as it probably affects a lot of generated code. I only made sure that all of the existing tests pass with the new spill instructions. Adding the test case from #106206 might make sense, although that code also only accounts for one very specific occurrence of a spill. What do you think? |
TechnoElf
force-pushed
the
m68k-movem-spill
branch
3 times, most recently
from
ff696e3 to
d9c0705
Compare
|
I've added some more test cases now. Do you think these adequately cover the issues addressed here? |
How about this one: ( The test.ll is the test provided in #106206) Also, we can also check from this snippet that the spill instruction is movem, because it stores to the spill slot (which is %stack.1) |
TechnoElf
force-pushed
the
m68k-movem-spill
branch
from
d9c0705 to
a42ca92
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
TechnoElf
force-pushed
the
m68k-movem-spill
branch
from
a42ca92 to
9841b85
Compare
|
@mshockwave Any chance this and #111145 could be landed? |
TechnoElf
force-pushed
the
m68k-movem-spill
branch
from
9841b85 to
3cdcef9
Compare
|
Thanks for the review! I've implemented your requested changes |
|
The commit message should have the "why" beyond linking to issue numbers. From my own research the "why" appears to be because MOVE will set condition codes, but MOVEM will not. It also appears you're conflating two changes here: one involving using MOVEM over MOVE, and one involving not promoting registers and overflowing the stack space allocated to them. |
|
@TechnoElf are you able to address the above feedback? Looks like it should just be documentation changes |
…32-bit 8-bit and 16-bit register spills were previously always promoted to 32-bit, despite only being assigned 16-bit stack slots. This resulted in unrelated data on the stack being overwritten. Fixes llvm#106209.
In the previous implementation, the mov instruction was used for 8-bit register spills. However, this instruction has the side effect of overwriting the CCR. When a spill is inserted between a compare and a branch instruction, this would cause the necessary condition flags to be overwritten. As 8-bit spills are already assigned 16-bit stack slots, the 16-bit movem instruction, which doesn't affect the CCR, may be used instead. Fixes llvm#106209.
TechnoElf
force-pushed
the
m68k-movem-spill
branch
from
3cdcef9 to
7a9809e
Compare
|
@jrtc27 @mshockwave @knickish I've split up the original commit and added more details to the new commit messages. |
|
@TechnoElf Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/12777 Here is the relevant piece of the build log for the reference |
|
Trying to repro this, may be spurious as this build doesn't even appear to target m68k |
|
Sanitizer tests fail from time to time, you can ignore this failure. |
Fixes #106206 and #106209.