[clang] d16a631 - [AVR] Merge AVRRelaxMemOperations into AVRExpandPseudoInsts

Ben Shi via cfe-commits cfe-commits at lists.llvm.org
Sun Apr 10 19:42:23 PDT 2022


Author: Patryk Wychowaniec
Date: 2022-04-11T02:42:13Z
New Revision: d16a631c124fdc27dd33037a826804ebf21dc582

URL: https://github.com/llvm/llvm-project/commit/d16a631c124fdc27dd33037a826804ebf21dc582
DIFF: https://github.com/llvm/llvm-project/commit/d16a631c124fdc27dd33037a826804ebf21dc582.diff

LOG: [AVR] Merge AVRRelaxMemOperations into AVRExpandPseudoInsts

This commit contains a refactoring that merges AVRRelaxMemOperations
into AVRExpandPseudoInsts, so that we have a single place in code that
expands the STDWPtrQRr opcode.

Seizing the day, I've also fixed a couple of potential bugs with our
previous implementation (e.g. when the destination register was killed,
the previous implementation would try to .addDef() that killed
register, crashing LLVM in the process - that's fixed now, as proved by
the test).

Reviewed By: benshi001

Differential Revision: https://reviews.llvm.org/D122533

Added: 
    

Modified: 
    clang/docs/tools/clang-formatted-files.txt
    llvm/lib/Target/AVR/AVR.h
    llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
    llvm/lib/Target/AVR/AVRTargetMachine.cpp
    llvm/lib/Target/AVR/CMakeLists.txt
    llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
    llvm/utils/gn/secondary/llvm/lib/Target/AVR/BUILD.gn

Removed: 
    llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp
    llvm/test/CodeGen/AVR/relax-mem/STDWPtrQRr.mir


################################################################################
diff  --git a/clang/docs/tools/clang-formatted-files.txt b/clang/docs/tools/clang-formatted-files.txt
index e0ce9b1c29228..e344c8faa7da2 100644
--- a/clang/docs/tools/clang-formatted-files.txt
+++ b/clang/docs/tools/clang-formatted-files.txt
@@ -6398,7 +6398,6 @@ llvm/lib/Target/AVR/AVRMCInstLower.cpp
 llvm/lib/Target/AVR/AVRMCInstLower.h
 llvm/lib/Target/AVR/AVRRegisterInfo.cpp
 llvm/lib/Target/AVR/AVRRegisterInfo.h
-llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp
 llvm/lib/Target/AVR/AVRSelectionDAGInfo.h
 llvm/lib/Target/AVR/AVRShiftExpand.cpp
 llvm/lib/Target/AVR/AVRSubtarget.cpp

diff  --git a/llvm/lib/Target/AVR/AVR.h b/llvm/lib/Target/AVR/AVR.h
index 532a57af25157..d29dc5f70e72e 100644
--- a/llvm/lib/Target/AVR/AVR.h
+++ b/llvm/lib/Target/AVR/AVR.h
@@ -29,12 +29,10 @@ FunctionPass *createAVRISelDag(AVRTargetMachine &TM,
                                CodeGenOpt::Level OptLevel);
 FunctionPass *createAVRExpandPseudoPass();
 FunctionPass *createAVRFrameAnalyzerPass();
-FunctionPass *createAVRRelaxMemPass();
 FunctionPass *createAVRBranchSelectionPass();
 
 void initializeAVRShiftExpandPass(PassRegistry &);
 void initializeAVRExpandPseudoPass(PassRegistry &);
-void initializeAVRRelaxMemPass(PassRegistry &);
 
 /// Contains the AVR backend.
 namespace AVR {

diff  --git a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
index de93c863abd86..22f91f98a1644 100644
--- a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
@@ -1157,32 +1157,51 @@ bool AVRExpandPseudo::expand<AVR::STWPtrPdRr>(Block &MBB, BlockIt MBBI) {
 template <>
 bool AVRExpandPseudo::expand<AVR::STDWPtrQRr>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;
-  Register SrcLoReg, SrcHiReg;
+
   Register DstReg = MI.getOperand(0).getReg();
-  Register SrcReg = MI.getOperand(2).getReg();
-  unsigned Imm = MI.getOperand(1).getImm();
   bool DstIsKill = MI.getOperand(0).isKill();
+  unsigned Imm = MI.getOperand(1).getImm();
+  Register SrcReg = MI.getOperand(2).getReg();
   bool SrcIsKill = MI.getOperand(2).isKill();
-  unsigned OpLo = AVR::STDPtrQRr;
-  unsigned OpHi = AVR::STDPtrQRr;
-  TRI->splitReg(SrcReg, SrcLoReg, SrcHiReg);
 
-  // Since we add 1 to the Imm value for the high byte below, and 63 is the
-  // highest Imm value allowed for the instruction, 62 is the limit here.
-  assert(Imm <= 62 && "Offset is out of range");
+  // STD's maximum displacement is 63, so larger stores have to be split into a
+  // set of operations
+  if (Imm >= 63) {
+    if (!DstIsKill) {
+      buildMI(MBB, MBBI, AVR::PUSHWRr).addReg(DstReg);
+    }
 
-  auto MIBLO = buildMI(MBB, MBBI, OpLo)
-                   .addReg(DstReg)
-                   .addImm(Imm)
-                   .addReg(SrcLoReg, getKillRegState(SrcIsKill));
+    buildMI(MBB, MBBI, AVR::SUBIWRdK)
+        .addReg(DstReg, RegState::Define)
+        .addReg(DstReg, RegState::Kill)
+        .addImm(-Imm);
 
-  auto MIBHI = buildMI(MBB, MBBI, OpHi)
-                   .addReg(DstReg, getKillRegState(DstIsKill))
-                   .addImm(Imm + 1)
-                   .addReg(SrcHiReg, getKillRegState(SrcIsKill));
+    buildMI(MBB, MBBI, AVR::STWPtrRr)
+        .addReg(DstReg, RegState::Kill)
+        .addReg(SrcReg, getKillRegState(SrcIsKill));
 
-  MIBLO.setMemRefs(MI.memoperands());
-  MIBHI.setMemRefs(MI.memoperands());
+    if (!DstIsKill) {
+      buildMI(MBB, MBBI, AVR::POPWRd).addDef(DstReg, RegState::Define);
+    }
+  } else {
+    unsigned OpLo = AVR::STDPtrQRr;
+    unsigned OpHi = AVR::STDPtrQRr;
+    Register SrcLoReg, SrcHiReg;
+    TRI->splitReg(SrcReg, SrcLoReg, SrcHiReg);
+
+    auto MIBLO = buildMI(MBB, MBBI, OpLo)
+                     .addReg(DstReg)
+                     .addImm(Imm)
+                     .addReg(SrcLoReg, getKillRegState(SrcIsKill));
+
+    auto MIBHI = buildMI(MBB, MBBI, OpHi)
+                     .addReg(DstReg, getKillRegState(DstIsKill))
+                     .addImm(Imm + 1)
+                     .addReg(SrcHiReg, getKillRegState(SrcIsKill));
+
+    MIBLO.setMemRefs(MI.memoperands());
+    MIBHI.setMemRefs(MI.memoperands());
+  }
 
   MI.eraseFromParent();
   return true;

diff  --git a/llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp b/llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp
deleted file mode 100644
index 76f29eb9f3697..0000000000000
--- a/llvm/lib/Target/AVR/AVRRelaxMemOperations.cpp
+++ /dev/null
@@ -1,144 +0,0 @@
-//===-- AVRRelaxMemOperations.cpp - Relax out of range loads/stores -------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file contains a pass which relaxes out of range memory operations into
-// equivalent operations which handle bigger addresses.
-//
-//===----------------------------------------------------------------------===//
-
-#include "AVR.h"
-#include "AVRInstrInfo.h"
-#include "AVRTargetMachine.h"
-#include "MCTargetDesc/AVRMCTargetDesc.h"
-
-#include "llvm/CodeGen/MachineFunctionPass.h"
-#include "llvm/CodeGen/MachineInstrBuilder.h"
-#include "llvm/CodeGen/MachineRegisterInfo.h"
-#include "llvm/CodeGen/TargetRegisterInfo.h"
-
-using namespace llvm;
-
-#define AVR_RELAX_MEM_OPS_NAME "AVR memory operation relaxation pass"
-
-namespace {
-
-class AVRRelaxMem : public MachineFunctionPass {
-public:
-  static char ID;
-
-  AVRRelaxMem() : MachineFunctionPass(ID) {
-    initializeAVRRelaxMemPass(*PassRegistry::getPassRegistry());
-  }
-
-  bool runOnMachineFunction(MachineFunction &MF) override;
-
-  StringRef getPassName() const override { return AVR_RELAX_MEM_OPS_NAME; }
-
-private:
-  typedef MachineBasicBlock Block;
-  typedef Block::iterator BlockIt;
-
-  const TargetInstrInfo *TII;
-
-  template <unsigned OP> bool relax(Block &MBB, BlockIt MBBI);
-
-  bool runOnBasicBlock(Block &MBB);
-  bool runOnInstruction(Block &MBB, BlockIt MBBI);
-
-  MachineInstrBuilder buildMI(Block &MBB, BlockIt MBBI, unsigned Opcode) {
-    return BuildMI(MBB, MBBI, MBBI->getDebugLoc(), TII->get(Opcode));
-  }
-};
-
-char AVRRelaxMem::ID = 0;
-
-bool AVRRelaxMem::runOnMachineFunction(MachineFunction &MF) {
-  bool Modified = false;
-
-  const AVRSubtarget &STI = MF.getSubtarget<AVRSubtarget>();
-  TII = STI.getInstrInfo();
-
-  for (Block &MBB : MF) {
-    bool BlockModified = runOnBasicBlock(MBB);
-    Modified |= BlockModified;
-  }
-
-  return Modified;
-}
-
-bool AVRRelaxMem::runOnBasicBlock(Block &MBB) {
-  bool Modified = false;
-
-  BlockIt MBBI = MBB.begin(), E = MBB.end();
-  while (MBBI != E) {
-    BlockIt NMBBI = std::next(MBBI);
-    Modified |= runOnInstruction(MBB, MBBI);
-    MBBI = NMBBI;
-  }
-
-  return Modified;
-}
-
-template <> bool AVRRelaxMem::relax<AVR::STDWPtrQRr>(Block &MBB, BlockIt MBBI) {
-  MachineInstr &MI = *MBBI;
-
-  MachineOperand &Ptr = MI.getOperand(0);
-  MachineOperand &Src = MI.getOperand(2);
-  int64_t Imm = MI.getOperand(1).getImm();
-
-  // We can definitely optimise this better.
-  if (Imm > 63) {
-    // Push the previous state of the pointer register.
-    // This instruction must preserve the value.
-    buildMI(MBB, MBBI, AVR::PUSHWRr).addReg(Ptr.getReg());
-
-    // Add the immediate to the pointer register.
-    buildMI(MBB, MBBI, AVR::SBCIWRdK)
-        .addReg(Ptr.getReg(), RegState::Define)
-        .addReg(Ptr.getReg())
-        .addImm(-Imm);
-
-    // Store the value in the source register to the address
-    // pointed to by the pointer register.
-    buildMI(MBB, MBBI, AVR::STWPtrRr)
-        .addReg(Ptr.getReg())
-        .addReg(Src.getReg(), getKillRegState(Src.isKill()));
-
-    // Pop the original state of the pointer register.
-    buildMI(MBB, MBBI, AVR::POPWRd)
-        .addDef(Ptr.getReg(), getKillRegState(Ptr.isKill()));
-
-    MI.removeFromParent();
-  }
-
-  return false;
-}
-
-bool AVRRelaxMem::runOnInstruction(Block &MBB, BlockIt MBBI) {
-  MachineInstr &MI = *MBBI;
-  int Opcode = MBBI->getOpcode();
-
-#define RELAX(Op)                                                              \
-  case Op:                                                                     \
-    return relax<Op>(MBB, MI)
-
-  switch (Opcode) { RELAX(AVR::STDWPtrQRr); }
-#undef RELAX
-  return false;
-}
-
-} // end of anonymous namespace
-
-INITIALIZE_PASS(AVRRelaxMem, "avr-relax-mem", AVR_RELAX_MEM_OPS_NAME, false,
-                false)
-
-namespace llvm {
-
-FunctionPass *createAVRRelaxMemPass() { return new AVRRelaxMem(); }
-
-} // end of namespace llvm

diff  --git a/llvm/lib/Target/AVR/AVRTargetMachine.cpp b/llvm/lib/Target/AVR/AVRTargetMachine.cpp
index 22b9ba3ece071..c8be3f150662d 100644
--- a/llvm/lib/Target/AVR/AVRTargetMachine.cpp
+++ b/llvm/lib/Target/AVR/AVRTargetMachine.cpp
@@ -92,7 +92,6 @@ extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAVRTarget() {
 
   auto &PR = *PassRegistry::getPassRegistry();
   initializeAVRExpandPseudoPass(PR);
-  initializeAVRRelaxMemPass(PR);
   initializeAVRShiftExpandPass(PR);
 }
 
@@ -118,7 +117,6 @@ bool AVRPassConfig::addInstSelector() {
 }
 
 void AVRPassConfig::addPreSched2() {
-  addPass(createAVRRelaxMemPass());
   addPass(createAVRExpandPseudoPass());
 }
 

diff  --git a/llvm/lib/Target/AVR/CMakeLists.txt b/llvm/lib/Target/AVR/CMakeLists.txt
index cde2138048dd4..ce8860ce2e3e4 100644
--- a/llvm/lib/Target/AVR/CMakeLists.txt
+++ b/llvm/lib/Target/AVR/CMakeLists.txt
@@ -22,7 +22,6 @@ add_llvm_target(AVRCodeGen
   AVRISelDAGToDAG.cpp
   AVRISelLowering.cpp
   AVRMCInstLower.cpp
-  AVRRelaxMemOperations.cpp
   AVRRegisterInfo.cpp
   AVRShiftExpand.cpp
   AVRSubtarget.cpp

diff  --git a/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir b/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
index 8c06bf8214ea8..fbec684516ee7 100644
--- a/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
+++ b/llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir
@@ -1,4 +1,4 @@
-# RUN: llc -O0 -run-pass=avr-expand-pseudo  %s -o - | FileCheck %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -verify-machineinstrs %s -o - | FileCheck %s
 
 --- |
   target triple = "avr--"
@@ -15,8 +15,52 @@ body: |
 
     ; CHECK-LABEL: test
 
-    ; CHECK:      STDPtrQRr $r29r28, 10, $r0
-    ; CHECK-NEXT: STDPtrQRr $r29r28, 11, $r1
+    ; Small displacement (<63):
+    ; CHECK:      STDPtrQRr $r29r28, 3, $r0
+    ; CHECK-NEXT: STDPtrQRr $r29r28, 4, $r1
+    STDWPtrQRr $r29r28, 3, $r1r0
 
-    STDWPtrQRr $r29r28, 10, $r1r0
+    ; Small displacement where the destination register is killed:
+    ; CHECK:      STDPtrQRr $r29r28, 3, $r0
+    ; CHECK-NEXT: STDPtrQRr killed $r29r28, 4, $r1
+    STDWPtrQRr killed $r29r28, 3, $r1r0
+
+    ; Small displacement where the source register is killed:
+    ; CHECK:      STDPtrQRr $r29r28, 3, killed $r0
+    ; CHECK-NEXT: STDPtrQRr $r29r28, 4, killed $r1
+    STDWPtrQRr $r29r28, 3, killed $r1r0
+
+    ; Small displacement, near the limit (=62):
+    ; CHECK:      STDPtrQRr $r29r28, 62, $r0
+    ; CHECK-NEXT: STDPtrQRr $r29r28, 63, $r1
+    STDWPtrQRr $r29r28, 62, $r1r0
+
+    ; Large displacement (>=63):
+    ; CHECK: PUSHRr $r28, implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: PUSHRr $r29, implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: $r28 = SUBIRdK killed $r28, 193, implicit-def $sreg
+    ; CHECK-NEXT: $r29 = SBCIRdK killed $r29, 255, implicit-def $sreg, implicit killed $sreg
+    ; CHECK-NEXT: STPtrRr $r29r28, $r0
+    ; CHECK-NEXT: STDPtrQRr $r29r28, 1, $r1
+    ; CHECK-NEXT: $r29 = POPRd implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: $r28 = POPRd implicit-def $sp, implicit $sp
+    STDWPtrQRr $r29r28, 63, $r1r0
+
+    ; Large displacement where the destination register is killed:
+    ; CHECK: $r28 = SUBIRdK killed $r28, 193, implicit-def $sreg
+    ; CHECK-NEXT: $r29 = SBCIRdK killed $r29, 255, implicit-def $sreg, implicit killed $sreg
+    ; CHECK-NEXT: STPtrRr $r29r28, $r0
+    ; CHECK-NEXT: STDPtrQRr $r29r28, 1, $r1
+    STDWPtrQRr killed $r29r28, 63, $r1r0
+
+    ; Large displacement where the source register is killed:
+    ; CHECK: PUSHRr $r28, implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: PUSHRr $r29, implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: $r28 = SUBIRdK killed $r28, 193, implicit-def $sreg
+    ; CHECK-NEXT: $r29 = SBCIRdK killed $r29, 255, implicit-def $sreg, implicit killed $sreg
+    ; CHECK-NEXT: STPtrRr $r29r28, killed $r0
+    ; CHECK-NEXT: STDPtrQRr $r29r28, 1, killed $r1
+    ; CHECK-NEXT: $r29 = POPRd implicit-def $sp, implicit $sp
+    ; CHECK-NEXT: $r28 = POPRd implicit-def $sp, implicit $sp
+    STDWPtrQRr $r29r28, 63, killed $r1r0
 ...

diff  --git a/llvm/test/CodeGen/AVR/relax-mem/STDWPtrQRr.mir b/llvm/test/CodeGen/AVR/relax-mem/STDWPtrQRr.mir
deleted file mode 100644
index 56f9e428f3651..0000000000000
--- a/llvm/test/CodeGen/AVR/relax-mem/STDWPtrQRr.mir
+++ /dev/null
@@ -1,31 +0,0 @@
-# RUN: llc -O0 -run-pass=avr-relax-mem %s -o - | FileCheck %s
-
---- |
-  target triple = "avr--"
-  define void @test() {
-  entry:
-    ret void
-  }
-...
-
----
-name:            test
-body: |
-  bb.0.entry:
-
-    ; CHECK-LABEL: test
-
-    ; We shouldn't expand things which already have 6-bit imms.
-    ; CHECK: STDWPtrQRr $r29r28, 63, $r1r0
-    STDWPtrQRr $r29r28, 63, $r1r0
-
-    ; We shouldn't expand things which already have 6-bit imms.
-    ; CHECK-NEXT: STDWPtrQRr $r29r28, 0, $r1r0
-    STDWPtrQRr $r29r28, 0, $r1r0
-
-    ; CHECK-NEXT: PUSHWRr $r29r28, implicit-def $sp, implicit $sp
-    ; CHECK-NEXT: $r29r28 = SBCIWRdK $r29r28, -64, implicit-def $sreg, implicit $sreg
-    ; CHECK-NEXT: STWPtrRr $r29r28, $r1r0
-    ; CHECK-NEXT: $r29r28 = POPWRd implicit-def $sp, implicit $sp
-    STDWPtrQRr $r29r28, 64, $r1r0
-...

diff  --git a/llvm/utils/gn/secondary/llvm/lib/Target/AVR/BUILD.gn b/llvm/utils/gn/secondary/llvm/lib/Target/AVR/BUILD.gn
index 223d1713a4981..28808d2cd0de0 100644
--- a/llvm/utils/gn/secondary/llvm/lib/Target/AVR/BUILD.gn
+++ b/llvm/utils/gn/secondary/llvm/lib/Target/AVR/BUILD.gn
@@ -37,7 +37,6 @@ static_library("LLVMAVRCodeGen") {
     "AVRInstrInfo.cpp",
     "AVRMCInstLower.cpp",
     "AVRRegisterInfo.cpp",
-    "AVRRelaxMemOperations.cpp",
     "AVRShiftExpand.cpp",
     "AVRSubtarget.cpp",
     "AVRTargetMachine.cpp",


        


More information about the cfe-commits mailing list