[AVR] Fix I/O instructions on XMEGA

[vlastik]

Summary

On XMEGA, I/O address space is same as data address space - there is no 0x20 offset,
because CPU General Purpose Registers are not mapped in data address space.

From https://en.wikipedia.org/wiki/AVR_microcontrollers

In the XMEGA variant, the working register file is not mapped into the data address space; as such, it is not possible to treat any of the XMEGA's working registers as though they were SRAM. Instead, the I/O registers are mapped into the data address space starting at the very beginning of the address space.

[vlastik]

Fixed 16bit accesses too

[dylanmckay]

Nice work, I didn't know of this particular XMEGA quirk.

I left a couple comments, then it looks good to merge.

Could you also add a CodeGen test to test/CodeGen/AVR/features for an XMEGA chip, along with a CHECK line that verifies that the correct assembly output is generated for XMEGA?

llvm/lib/Target/AVR/AVRDevices.td

127

add an extra s to addres

llvm/lib/Target/AVR/AVRInstrInfo.td

110–112

Rather than hardcoding these magic constants in this file, recommend adding a new method to AVRSubtarget, like uint8_t getIoRegisterOffset() and moving the ternary logic there.

Reuse this method in the below PatLeafs

[vlastik]

Fixed typo (missing s in address)
Moved magic logic to function in AVRSubtarget
Implemented a CodeGen test

[dylanmckay]

Really nice work, it's good to go.

[vlastik]

In D77207#1990945, @dylanmckay wrote:

Really nice work, it's good to go.

Thanks, then I need someone to commit the patch, please

[dylanmckay]

@vlastik Merged into master in 1420f4efbe7ca34355b2dd85d396197d92cd5e3f.

Thanks again for the patch!

[RKSimon]

llvm/lib/Target/AVR/AVRInstrInfo.td

146

@vlastik the "val >= 0x0" is redundant as these are unsigned values and gcc warns with:

lib/Target/AVR/AVRGenDAGISel.inc:1676:14: error: comparison of unsigned expression >= 0 is always true [-Werror=type-limits]
   return val >= 0x0 && val < 0x3f;

which causes problems with Werror builds - can these be removed please?

[RKSimon]

llvm/lib/Target/AVR/AVRInstrInfo.td

146

@dylanmckay @vlastik ping

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">;
 
+// 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 addres space">;
+
 class ELFArch<string name>  : SubtargetFeature<"", "ELFArch",
                                     !strconcat("ELF::",name), "">;
 
 // ELF e_flags architecture values
 def ELFArchAVR1    : ELFArch<"EF_AVR_ARCH_AVR1">;
 def ELFArchAVR2    : ELFArch<"EF_AVR_ARCH_AVR2">;
 def ELFArchAVR25   : ELFArch<"EF_AVR_ARCH_AVR25">;
 def ELFArchAVR3    : ELFArch<"EF_AVR_ARCH_AVR3">;

 //===---------------------------------------------------------------------===//
 // AVR Families
 //===---------------------------------------------------------------------===//
 
 // The device has at least the bare minimum that **every** single AVR
 // device should have.
 def FamilyAVR0           : Family<"avr0", []>;
 
-def FamilyAVR1           : Family<"avr1", [FamilyAVR0, FeatureLPM]>;
+def FamilyAVR1           : Family<"avr1", [FamilyAVR0, FeatureLPM, FeatureMMR]>;
 
 def FamilyAVR2           : Family<"avr2",
                                  [FamilyAVR1, FeatureIJMPCALL, FeatureADDSUBIW,
                                   FeatureSRAM]>;
 
 def FamilyAVR25          : Family<"avr25",
                                  [FamilyAVR2, FeatureMOVW, FeatureLPMX,
                                   FeatureSPM, FeatureBREAK]>;

 def FamilyAVR51          : Family<"avr51",
                                  [FamilyAVR5, FeatureELPM, FeatureELPMX]>;
 
 def FamilyAVR6           : Family<"avr6",
                                  [FamilyAVR51]>;
 
 def FamilyTiny           : Family<"avrtiny",
                                  [FamilyAVR0, FeatureBREAK, FeatureSRAM,
-                                  FeatureTinyEncoding]>;
+                                  FeatureTinyEncoding, FeatureMMR]>;
 
 def FamilyXMEGA          : Family<"xmega",
-                                 [FamilyAVR51, FeatureEIJMPCALL, FeatureSPMX,
-                                  FeatureDES]>;
+                                 [FamilyAVR0, FeatureLPM, FeatureIJMPCALL, FeatureADDSUBIW,
+                                  FeatureSRAM, FeatureJMPCALL, FeatureMultiplication,
+                                  FeatureMOVW, FeatureLPMX, FeatureSPM,
+                                  FeatureBREAK, FeatureEIJMPCALL, FeatureSPMX,
+                                  FeatureDES, FeatureELPM, FeatureELPMX]>;
 
 def FamilyXMEGAU         : Family<"xmegau",
                                   [FamilyXMEGA, FeatureRMW]>;
 
 def FeatureSetSpecial    : FeatureSet<"special",
                                       "Enable use of the entire instruction "
                                       "set - used for debugging",
                                       [FeatureSRAM, FeatureJMPCALL,
                                        FeatureIJMPCALL, FeatureEIJMPCALL,
                                        FeatureADDSUBIW, FeatureMOVW,
                                        FeatureLPM, FeatureLPMX, FeatureELPM,
                                        FeatureELPMX, FeatureSPM, FeatureSPMX,
                                        FeatureDES, FeatureRMW,
-                                       FeatureMultiplication, FeatureBREAK]>;
+                                       FeatureMultiplication, FeatureBREAK, FeatureMMR]>;
 
 //===---------------------------------------------------------------------===//
 // AVR microcontrollers supported.
 //===---------------------------------------------------------------------===//
 
 class Device<string Name, Family Fam, ELFArch Arch,
              list<SubtargetFeature> ExtraFeatures = []>
   : Processor<Name, NoItineraries, !listconcat([Fam,Arch],ExtraFeatures)>;

llvm/lib/Target/AVR/AVRInstrInfo.td

 // is handled by the destination instructions, which use imm_com8.
 def imm_com8_asmoperand : AsmOperandClass { let Name = "ImmCom8"; }
 def imm_com8 : Operand<i8> {
   let ParserMatchClass = imm_com8_asmoperand;
 }
 
 def ioaddr_XFORM : SDNodeXForm<imm,
 [{
-  return CurDAG->getTargetConstant(uint8_t(N->getZExtValue()) - 0x20, SDLoc(N), MVT::i8);
+  uint8_t offset = Subtarget->hasMemMappedGPR() ? 0x20 : 0x0;
+  return CurDAG->getTargetConstant(uint8_t(N->getZExtValue()) - offset,
+                                   SDLoc(N), MVT::i8);
 }]>;
 
 def iobitpos8_XFORM : SDNodeXForm<imm,
 [{
   return CurDAG->getTargetConstant(Log2_32(uint8_t(N->getZExtValue())),
                                    SDLoc(N), MVT::i8);
 }]>;
 
 def iobitposn8_XFORM : SDNodeXForm<imm,
 [{
   return CurDAG->getTargetConstant(Log2_32(uint8_t(~N->getZExtValue())),
                                    SDLoc(N), MVT::i8);
 }]>;
 
 def ioaddr8 : PatLeaf<(imm),
 [{
-  uint64_t val = N->getZExtValue();
-  return val >= 0x20 && val < 0x60;
+  uint8_t offset = Subtarget->hasMemMappedGPR() ? 0x20 : 0x0;
+  uint64_t val = N->getZExtValue() - offset;
+  return val >= 0x0 && val < 0x40;
 }], ioaddr_XFORM>;
 
 def lowioaddr8 : PatLeaf<(imm),
 [{
-  uint64_t val = N->getZExtValue();
-  return val >= 0x20 && val < 0x40;
+  uint8_t offset = Subtarget->hasMemMappedGPR() ? 0x20 : 0x0;
+  uint64_t val = N->getZExtValue() - offset;
+  return val >= 0x0 && val < 0x20;
 }], ioaddr_XFORM>;
 
 def ioaddr16 : PatLeaf<(imm),
 [{
   uint64_t val = N->getZExtValue();
   return val >= 0x20 && val < 0x5f;
 }], ioaddr_XFORM>;
 
 def iobitpos8 : PatLeaf<(imm),
 [{
   return isPowerOf2_32(uint8_t(N->getZExtValue()));
 }], iobitpos8_XFORM>;
 
 def iobitposn8 : PatLeaf<(imm),
 [{
   return isPowerOf2_32(uint8_t(~N->getZExtValue()));

llvm/lib/Target/AVR/AVRSubtarget.h

public:
   bool hasELPMX() const { return m_hasELPMX; }
   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; }
 
   /// 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");
     return ELFArch;
   }
private:
   bool m_hasELPMX;
   bool m_hasSPM;
   bool m_hasSPMX;
   bool m_hasDES;
   bool m_supportsRMW;
   bool m_supportsMultiplication;
   bool m_hasBREAK;
   bool m_hasTinyEncoding;
+  bool m_hasMemMappedGPR;
 
   // 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;
 
   AVRInstrInfo InstrInfo;
   AVRFrameLowering FrameLowering;
   AVRTargetLowering TLInfo;
   AVRSelectionDAGInfo TSInfo;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_SUBTARGET_H

llvm/lib/Target/AVR/AVRSubtarget.cpp

 #define GET_SUBTARGETINFO_TARGET_DESC
 #define GET_SUBTARGETINFO_CTOR
 #include "AVRGenSubtargetInfo.inc"
 
 namespace llvm {
 
 AVRSubtarget::AVRSubtarget(const Triple &TT, const std::string &CPU,
                            const std::string &FS, const AVRTargetMachine &TM)
-    : AVRGenSubtargetInfo(TT, CPU, FS),
-      ELFArch(0),
+    : AVRGenSubtargetInfo(TT, CPU, FS), ELFArch(0),
 
       // Subtarget features
       m_hasSRAM(false), m_hasJMPCALL(false), m_hasIJMPCALL(false),
       m_hasEIJMPCALL(false), m_hasADDSUBIW(false), m_hasSmallStack(false),
-      m_hasMOVW(false), m_hasLPM(false), m_hasLPMX(false),  m_hasELPM(false),
+      m_hasMOVW(false), m_hasLPM(false), m_hasLPMX(false), m_hasELPM(false),
       m_hasELPMX(false), m_hasSPM(false), m_hasSPMX(false), m_hasDES(false),
       m_supportsRMW(false), m_supportsMultiplication(false), m_hasBREAK(false),
-      m_hasTinyEncoding(false), m_FeatureSetDummy(false),
+      m_hasTinyEncoding(false), m_hasMemMappedGPR(false),
+      m_FeatureSetDummy(false),
 
       InstrInfo(), FrameLowering(),
       TLInfo(TM, initializeSubtargetDependencies(CPU, FS, TM)), TSInfo() {
   // Parse features string.
   ParseSubtargetFeatures(CPU, FS);
 }
 
 AVRSubtarget &
 AVRSubtarget::initializeSubtargetDependencies(StringRef CPU, StringRef FS,
                                               const TargetMachine &TM) {
   // Parse features string.
   ParseSubtargetFeatures(CPU, FS);
   return *this;
 }
 
 } // end of namespace llvm