From da0fe5db999baa659c2e386e5b0636dadfbbf759 Mon Sep 17 00:00:00 2001 From: Jim Lin Date: Mon, 23 Dec 2019 11:24:20 +0800 Subject: [PATCH] [AVR] Fix codegen for rotate instructions Summary: This patch introduces the ROLBRd and RORBRd pseudo-instructions, which implemenent the "traditional" rotate operations; instead of the AVR rotate instructions that use the carry bit. The code is not optimized at all. Especially when dealing with loops of rotate instructions, this codegen should be improved some day. Related bug: 41358 //Note//: This is my first submitted patch. Reviewers: dylanmckay, Jim Reviewed By: dylanmckay Subscribers: hiraditya, llvm-commits, dylanmckay, dsprenkels Tags: #llvm Patched by dsprenkels (Daan Sprenkels) Differential Revision: https://reviews.llvm.org/D60365 --- llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp | 91 ++++++++++++++++++++++++++++ llvm/lib/Target/AVR/AVRISelLowering.cpp | 5 +- llvm/lib/Target/AVR/AVRInstrInfo.td | 12 +++- llvm/test/CodeGen/AVR/rot.ll | 8 ++- 4 files changed, 110 insertions(+), 6 deletions(-) diff --git a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp index 83d0f68..f466c5c 100644 --- a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp +++ b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp @@ -52,6 +52,8 @@ private: /// The register to be used for temporary storage. const unsigned SCRATCH_REGISTER = AVR::R0; + /// The register that will always contain zero. + const unsigned ZERO_REGISTER = AVR::R1; /// The IO address of the status register. const unsigned SREG_ADDR = 0x3f; @@ -1243,6 +1245,93 @@ bool AVRExpandPseudo::expand(Block &MBB, BlockIt MBBI) { } template <> +bool AVRExpandPseudo::expand(Block &MBB, BlockIt MBBI) { + // In AVR, the rotate instructions behave quite unintuitively. They rotate + // bits through the carry bit in SREG, effectively rotating over 9 bits, + // instead of 8. This is useful when we are dealing with numbers over + // multiple registers, but when we actually need to rotate stuff, we have + // to explicitly add the carry bit. + + MachineInstr &MI = *MBBI; + unsigned OpShift, OpCarry; + unsigned DstReg = MI.getOperand(0).getReg(); + bool DstIsDead = MI.getOperand(0).isDead(); + OpShift = AVR::ADDRdRr; + OpCarry = AVR::ADCRdRr; + + // add r16, r16 + // adc r16, r1 + + // Shift part + buildMI(MBB, MBBI, OpShift) + .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead)) + .addReg(DstReg) + .addReg(DstReg); + + // Add the carry bit + auto MIB = buildMI(MBB, MBBI, OpCarry) + .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead)) + .addReg(DstReg) + .addReg(ZERO_REGISTER); + + // SREG is always implicitly killed + MIB->getOperand(2).setIsKill(); + + MI.eraseFromParent(); + return true; +} + +template <> +bool AVRExpandPseudo::expand(Block &MBB, BlockIt MBBI) { + // In AVR, the rotate instructions behave quite unintuitively. They rotate + // bits through the carry bit in SREG, effectively rotating over 9 bits, + // instead of 8. This is useful when we are dealing with numbers over + // multiple registers, but when we actually need to rotate stuff, we have + // to explicitly add the carry bit. + + MachineInstr &MI = *MBBI; + unsigned OpShiftOut, OpLoad, OpShiftIn, OpAdd; + unsigned DstReg = MI.getOperand(0).getReg(); + bool DstIsDead = MI.getOperand(0).isDead(); + OpShiftOut = AVR::LSRRd; + OpLoad = AVR::LDIRdK; + OpShiftIn = AVR::RORRd; + OpAdd = AVR::ORRdRr; + + // lsr r16 + // ldi r0, 0 + // ror r0 + // or r16, r17 + + // Shift out + buildMI(MBB, MBBI, OpShiftOut) + .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead)) + .addReg(DstReg); + + // Put 0 in temporary register + buildMI(MBB, MBBI, OpLoad) + .addReg(SCRATCH_REGISTER, RegState::Define | getDeadRegState(true)) + .addImm(0x00); + + // Shift in + buildMI(MBB, MBBI, OpShiftIn) + .addReg(SCRATCH_REGISTER, RegState::Define | getDeadRegState(true)) + .addReg(SCRATCH_REGISTER); + + // Add the results together using an or-instruction + auto MIB = buildMI(MBB, MBBI, OpAdd) + .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead)) + .addReg(DstReg) + .addReg(SCRATCH_REGISTER); + + // SREG is always implicitly killed + MIB->getOperand(2).setIsKill(); + + MI.eraseFromParent(); + return true; +} + +template <> bool AVRExpandPseudo::expand(Block &MBB, BlockIt MBBI) { MachineInstr &MI = *MBBI; unsigned OpLo, OpHi, DstLoReg, DstHiReg; @@ -1562,6 +1651,8 @@ bool AVRExpandPseudo::expandMI(Block &MBB, BlockIt MBBI) { EXPAND(AVR::OUTWARr); EXPAND(AVR::PUSHWRr); EXPAND(AVR::POPWRd); + EXPAND(AVR::ROLBRd); + EXPAND(AVR::RORBRd); EXPAND(AVR::LSLWRd); EXPAND(AVR::LSRWRd); EXPAND(AVR::RORWRd); diff --git a/llvm/lib/Target/AVR/AVRISelLowering.cpp b/llvm/lib/Target/AVR/AVRISelLowering.cpp index f12c59b..4dd843d 100644 --- a/llvm/lib/Target/AVR/AVRISelLowering.cpp +++ b/llvm/lib/Target/AVR/AVRISelLowering.cpp @@ -1472,16 +1472,15 @@ MachineBasicBlock *AVRTargetLowering::insertShift(MachineInstr &MI, RC = &AVR::DREGSRegClass; break; case AVR::Rol8: - Opc = AVR::ADCRdRr; // ROL is an alias of ADC Rd, Rd + Opc = AVR::ROLBRd; RC = &AVR::GPR8RegClass; - HasRepeatedOperand = true; break; case AVR::Rol16: Opc = AVR::ROLWRd; RC = &AVR::DREGSRegClass; break; case AVR::Ror8: - Opc = AVR::RORRd; + Opc = AVR::RORBRd; RC = &AVR::GPR8RegClass; break; case AVR::Ror16: diff --git a/llvm/lib/Target/AVR/AVRInstrInfo.td b/llvm/lib/Target/AVR/AVRInstrInfo.td index caca9b6..3de28ea 100644 --- a/llvm/lib/Target/AVR/AVRInstrInfo.td +++ b/llvm/lib/Target/AVR/AVRInstrInfo.td @@ -1676,6 +1676,16 @@ Defs = [SREG] in { // 8-bit ROL is an alias of ADC Rd, Rd + def ROLBRd : Pseudo<(outs GPR8:$rd), + (ins GPR8:$src), + "rolb\t$rd", + [(set i8:$rd, (AVRrol i8:$src)), (implicit SREG)]>; + + def RORBRd : Pseudo<(outs GPR8:$rd), + (ins GPR8:$src), + "rorb\t$rd", + [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>; + def ROLWRd : Pseudo<(outs DREGS:$rd), (ins DREGS:$src), "rolw\t$rd", @@ -1686,7 +1696,7 @@ Defs = [SREG] in (outs GPR8:$rd), (ins GPR8:$src), "ror\t$rd", - [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>; + []>; def RORWRd : Pseudo<(outs DREGS:$rd), (ins DREGS:$src), diff --git a/llvm/test/CodeGen/AVR/rot.ll b/llvm/test/CodeGen/AVR/rot.ll index a7b77d9..49981aa 100644 --- a/llvm/test/CodeGen/AVR/rot.ll +++ b/llvm/test/CodeGen/AVR/rot.ll @@ -10,7 +10,8 @@ define i8 @rol8(i8 %val, i8 %amt) { ; CHECK-NEXT: breq LBB0_2 ; CHECK-NEXT: LBB0_1: - ; CHECK-NEXT: rol r24 + ; CHECK-NEXT: lsl r24 + ; CHECK-NEXT: adc r24, r1 ; CHECK-NEXT: subi r22, 1 ; CHECK-NEXT: brne LBB0_1 @@ -36,7 +37,10 @@ define i8 @ror8(i8 %val, i8 %amt) { ; CHECK-NEXT: breq LBB1_2 ; CHECK-NEXT: LBB1_1: - ; CHECK-NEXT: ror r24 + ; CHECK-NEXT: lsr r24 + ; CHECK-NEXT: ldi r0, 0 + ; CHECK-NEXT: ror r0 + ; CHECK-NEXT: or r24, r0 ; CHECK-NEXT: subi r22, 1 ; CHECK-NEXT: brne LBB1_1 -- 2.7.4