[AVR] Fix an issue of writing 16-bit ports

[benshi001]

Summary

For 16-bit ports, the normal devices reqiure writing high byte first
and then low byte. But the XMEGA devices require the reverse order.

Fixes https://github.com/llvm/llvm-project/issues/58395

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1294

I like this form

Low bound <= variable && variable <= high bound.

Rather then

variable' <= Low bound` && variable <= high bound.

which looks like variable is in range [low, high].

[aykevl]

This works for the out instruction, but what about a regular volatile store? I think it should be changed in the same way.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1292–1293

I think this is usually done via subtarget flags, not by looking at e_flags. I couldn't easily find something similar in other targets. But this also works.

[benshi001]

In D141752#4065457, @aykevl wrote:

This works for the out instruction, but what about a regular volatile store? I think it should be changed in the same way.

I have done for both OUTW and STSWKRr. Thanks!

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1292–1293

I have added a new target feature FeatureLowByteFirst and all XMEGA families contains this new feature.

[benshi001]

llvm/lib/Target/AVR/AVRISelLowering.cpp

1114 ↗

(On Diff #491038)

We disallow post increment store for 16-bit store on ordinary AVRs, while the XMEGA devices are still allowed.

[benshi001]

All four pseudo instructions are modified : OUTW, STSWKRr, STDWPtrQRr, STWPtrRr.

[benshi001]

Unfortunately many tests are affected, fortunately the only change is exchanging the order of a store pair. ^_^

[benshi001]

We can see the full affection in the four pseudo instruction's .mir tests.

[benshi001]

llvm/test/CodeGen/AVR/pseudo/STDWPtrQRr.mir

24 ↗

(On Diff #491038)

The only difference between CHECK and CHECK-XMEGA is the order.

[benshi001]

ping ...

[benshi001]

ping ...

[benshi001]

ping ...

[jacquesguan]

LGTM.

llvm/lib/Target/AVR/AVRDevices.td

def FeatureBREAK : SubtargetFeature<"break", "m_hasBREAK", "true",
                                     "instruction">;
 
 // The device has instruction encodings specific to the Tiny core.
 def FeatureTinyEncoding
     : SubtargetFeature<"tinyencoding", "m_hasTinyEncoding", "true",
                        "The device has Tiny core specific "
                        "instruction encodings">;
 
+// When writing a 16-bit port or storing a 16-bit word, do the low byte first.
+def FeatureLowByteFirst
+    : SubtargetFeature<"lowbytefirst", "m_hasLowByteFirst", "true",
+                       "Do the low byte first when writing a 16-bit port or "
+                       "storing a 16-bit word">;
+
 // The device has CPU registers mapped in data address space
 def FeatureMMR : SubtargetFeature<"memmappedregs", "m_hasMemMappedGPR", "true",
                                   "The device has CPU registers "
                                   "mapped in data address space">;
 
 class ELFArch<string name>
     : SubtargetFeature<"", "ELFArch", !strconcat("ELF::", name), "">;
 
def FamilyTiny
     : Family<"avrtiny",
              [FamilyAVR0, FeatureBREAK, FeatureSRAM, FeatureTinyEncoding,
               FeatureSmallStack]>;
 
 def FamilyXMEGA3 : Family<"xmega3",
                           [FamilyAVR0, FeatureLPM, FeaturePROGMEM, FeatureIJMPCALL,
                            FeatureADDSUBIW, FeatureSRAM, FeatureJMPCALL,
                            FeatureMultiplication, FeatureMOVW, FeatureLPMX,
-                           FeatureBREAK]>;
+                           FeatureBREAK, FeatureLowByteFirst]>;
 
 def FamilyXMEGA : Family<"xmega",
                          [FamilyAVR0, FeatureLPM, FeaturePROGMEM, FeatureIJMPCALL,
                           FeatureADDSUBIW, FeatureSRAM, FeatureJMPCALL,
                           FeatureMultiplication, FeatureMOVW, FeatureLPMX,
                           FeatureSPM, FeatureBREAK, FeatureEIJMPCALL,
-                          FeatureSPMX, FeatureDES, FeatureELPM, FeatureELPMX]>;
+                          FeatureSPMX, FeatureDES, FeatureELPM, FeatureELPMX,
+                          FeatureLowByteFirst]>;
 
 def FamilyXMEGAU : Family<"xmegau", [FamilyXMEGA, FeatureRMW]>;
 
 def FeatureSetSpecial
     : FeatureSet<"special",
                  "Enable use of the entire instruction "
                  "set - used for debugging",
                  [

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

 bool AVRExpandPseudo::expand<AVR::AtomicFence>(Block &MBB, BlockIt MBBI) {
   // On AVR, there is only one core and so atomic fences do nothing.
   MBBI->eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::STSWKRr>(Block &MBB, BlockIt MBBI) {
+  const AVRSubtarget &STI = MBB.getParent()->getSubtarget<AVRSubtarget>();
   MachineInstr &MI = *MBBI;
   Register SrcLoReg, SrcHiReg;
   Register SrcReg = MI.getOperand(1).getReg();
   bool SrcIsKill = MI.getOperand(1).isKill();
-  unsigned OpLo = AVR::STSKRr;
-  unsigned OpHi = AVR::STSKRr;
   TRI->splitReg(SrcReg, SrcLoReg, SrcHiReg);
 
-  // Write the high byte first in case this address belongs to a special
-  // I/O address with a special temporary register.
-  auto MIBHI = buildMI(MBB, MBBI, OpHi);
-  auto MIBLO = buildMI(MBB, MBBI, OpLo);
+  auto MIB0 = buildMI(MBB, MBBI, AVR::STSKRr);
+  auto MIB1 = buildMI(MBB, MBBI, AVR::STSKRr);
 
   switch (MI.getOperand(0).getType()) {
   case MachineOperand::MO_GlobalAddress: {
     const GlobalValue *GV = MI.getOperand(0).getGlobal();
     int64_t Offs = MI.getOperand(0).getOffset();
     unsigned TF = MI.getOperand(0).getTargetFlags();
 
-    MIBLO.addGlobalAddress(GV, Offs, TF);
-    MIBHI.addGlobalAddress(GV, Offs + 1, TF);
+    if (STI.hasLowByteFirst()) {
+      // Write the low byte first for XMEGA devices.
+      MIB0.addGlobalAddress(GV, Offs, TF);
+      MIB1.addGlobalAddress(GV, Offs + 1, TF);
+    } else {
+      // Write the high byte first for traditional devices.
+      MIB0.addGlobalAddress(GV, Offs + 1, TF);
+      MIB1.addGlobalAddress(GV, Offs, TF);
+    }
+
     break;
   }
   case MachineOperand::MO_Immediate: {
     unsigned Imm = MI.getOperand(0).getImm();
 
-    MIBLO.addImm(Imm);
-    MIBHI.addImm(Imm + 1);
+    if (STI.hasLowByteFirst()) {
+      // Write the low byte first for XMEGA devices.
+      MIB0.addImm(Imm);
+      MIB1.addImm(Imm + 1);
+    } else {
+      // Write the high byte first for traditional devices.
+      MIB0.addImm(Imm + 1);
+      MIB1.addImm(Imm);
+    }
+
     break;
   }
   default:
     llvm_unreachable("Unknown operand type!");
   }
 
-  MIBLO.addReg(SrcLoReg, getKillRegState(SrcIsKill));
-  MIBHI.addReg(SrcHiReg, getKillRegState(SrcIsKill));
-
-  MIBLO.setMemRefs(MI.memoperands());
-  MIBHI.setMemRefs(MI.memoperands());
+  if (STI.hasLowByteFirst()) {
+    // Write the low byte first for XMEGA devices.
+    MIB0.addReg(SrcLoReg, getKillRegState(SrcIsKill))
+        .setMemRefs(MI.memoperands());
+    MIB1.addReg(SrcHiReg, getKillRegState(SrcIsKill))
+        .setMemRefs(MI.memoperands());
+  } else {
+    // Write the high byte first for traditional devices.
+    MIB0.addReg(SrcHiReg, getKillRegState(SrcIsKill))
+        .setMemRefs(MI.memoperands());
+    MIB1.addReg(SrcLoReg, getKillRegState(SrcIsKill))
+        .setMemRefs(MI.memoperands());
+  }
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::STWPtrRr>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;
bool AVRExpandPseudo::expand<AVR::INWRdA>(Block &MBB, BlockIt MBBI) {
   MIBHI.setMemRefs(MI.memoperands());
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::OUTWARr>(Block &MBB, BlockIt MBBI) {
+  const AVRSubtarget &STI = MBB.getParent()->getSubtarget<AVRSubtarget>();
   MachineInstr &MI = *MBBI;
   Register SrcLoReg, SrcHiReg;
   unsigned Imm = MI.getOperand(0).getImm();
   Register SrcReg = MI.getOperand(1).getReg();
   bool SrcIsKill = MI.getOperand(1).isKill();
-  unsigned OpLo = AVR::OUTARr;
-  unsigned OpHi = AVR::OUTARr;
   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 && "Address is out of range");
 
-  // 16 bit I/O writes need the high byte first
-  auto MIBHI = buildMI(MBB, MBBI, OpHi)
-                   .addImm(Imm + 1)
-                   .addReg(SrcHiReg, getKillRegState(SrcIsKill));
-
-  auto MIBLO = buildMI(MBB, MBBI, OpLo)
-                   .addImm(Imm)
-                   .addReg(SrcLoReg, getKillRegState(SrcIsKill));
+  // 16 bit I/O writes need the high byte first on normal AVR devices,
+  // and in reverse order for the XMEGA/XMEGA3/XMEGAU families.
+  auto MIBHI = buildMI(MBB, MBBI, AVR::OUTARr)
+                   .addImm(STI.hasLowByteFirst() ? Imm : Imm + 1)
+                   .addReg(STI.hasLowByteFirst() ? SrcLoReg : SrcHiReg,
+                           getKillRegState(SrcIsKill));
+  auto MIBLO = buildMI(MBB, MBBI, AVR::OUTARr)
+                   .addImm(STI.hasLowByteFirst() ? Imm + 1 : Imm)
+                   .addReg(STI.hasLowByteFirst() ? SrcHiReg : SrcLoReg,
+                           getKillRegState(SrcIsKill));
 
   MIBLO.setMemRefs(MI.memoperands());
   MIBHI.setMemRefs(MI.memoperands());
 
   MI.eraseFromParent();
   return true;
 }
 

llvm/lib/Target/AVR/AVRSubtarget.h

public:
   bool hasSPM() const { return m_hasSPM; }
   bool hasSPMX() const { return m_hasSPMX; }
   bool hasDES() const { return m_hasDES; }
   bool supportsRMW() const { return m_supportsRMW; }
   bool supportsMultiplication() const { return m_supportsMultiplication; }
   bool hasBREAK() const { return m_hasBREAK; }
   bool hasTinyEncoding() const { return m_hasTinyEncoding; }
   bool hasMemMappedGPR() const { return m_hasMemMappedGPR; }
+  bool hasLowByteFirst() const { return m_hasLowByteFirst; }
 
   uint8_t getIORegisterOffset() const { return hasMemMappedGPR() ? 0x20 : 0x0; }
 
   /// Gets the ELF architecture for the e_flags field
   /// of an ELF object file.
   unsigned getELFArch() const {
     assert(ELFArch != 0 &&
            "every device must have an associate ELF architecture");
private:
   bool m_hasPROGMEM = false;
   bool m_hasSPM = false;
   bool m_hasSPMX = false;
   bool m_hasDES = false;
   bool m_supportsRMW = false;
   bool m_supportsMultiplication = false;
   bool m_hasBREAK = false;
   bool m_hasTinyEncoding = false;
+  bool m_hasLowByteFirst = false;
   bool m_hasMemMappedGPR = false;
 
   // Dummy member, used by FeatureSet's. We cannot have a SubtargetFeature with
   // no variable, so we instead bind pseudo features to this variable.
   bool m_FeatureSetDummy = false;
 
   AVRInstrInfo InstrInfo;
   AVRFrameLowering FrameLowering;
   AVRTargetLowering TLInfo;
   AVRSelectionDAGInfo TSInfo;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_SUBTARGET_H

llvm/test/CodeGen/AVR/pseudo/OUTWARr.mir

-# RUN: llc -O0 -run-pass=avr-expand-pseudo %s -o - | FileCheck %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -mtriple=avr -mcpu=attiny11 %s -o - \
+# RUN:     | FileCheck %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -mtriple=avr -mcpu=atmega328 %s -o - \
+# RUN:     | FileCheck %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -mtriple=avr -mcpu=attiny817 %s -o - \
+# RUN:     | FileCheck --check-prefix=XMEGA %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -mtriple=avr -mcpu=atxmega64a1 %s -o - \
+# RUN:     | FileCheck --check-prefix=XMEGA %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -mtriple=avr -mcpu=atxmega256a3u %s -o - \
+# RUN:     | FileCheck --check-prefix=XMEGA %s
 
 --- |
   target triple = "avr--"
   define void @test() {
   entry:
     ret void
   }
 ...
 
 ---
 name:            test
 body: |
   bb.0.entry:
     liveins: $r15r14
 
     ; CHECK-LABEL: test
-
-    ; CHECK:      OUTARr 32, $r15
-    ; CHECK-NEXT: OUTARr 31, $r14
+    ; CHECK:       OUTARr 32, $r15
+    ; CHECK-NEXT:  OUTARr 31, $r14
 
+    ; XMEGA-LABEL: test
+    ; XMEGA:       OUTARr 31, $r14
+    ; XMEGA-NEXT:  OUTARr 32, $r15
+
     OUTWARr 31, $r15r14
 ...

llvm/test/CodeGen/AVR/pseudo/STSWKRr.mir

 # RUN: llc -O0 -run-pass=avr-expand-pseudo %s -o - | FileCheck %s
+# RUN: llc -O0 -run-pass=avr-expand-pseudo -mcpu=atxmega64a1 %s -o - \
+# RUN:     | FileCheck --check-prefix=XMEGA %s
 
 # This test checks the expansion of the 16-bit STSWRdK pseudo instruction.
 
 --- |
   target triple = "avr--"
   define void @test_stswkrr() {
   entry:
     ret void
   }
 ...
 
 ---
 name:            test_stswkrr
 body: |
   bb.0.entry:
     liveins: $r31r30
 
     ; CHECK-LABEL: test_stswkrr
-
-    ; CHECK:      STSKRr 2560, $r31
-    ; CHECK-NEXT: STSKRr 2559, $r30
+    ; CHECK:       STSKRr 2560, $r31
+    ; CHECK-NEXT:  STSKRr 2559, $r30
 
+    ; XMEGA-LABEL: test_stswkrr
+    ; XMEGA:       STSKRr 2559, $r30
+    ; XMEGA-NEXT:  STSKRr 2560, $r31
+
     STSWKRr 2559, $r31r30
 ...