[AVR][MC] Generate section '.progmemX.data' for extended flash banks

[benshi001]

Some design notes

1. Input address space 1 corresponds to __flash (ordinrary flash < 64KB), addrspace 2 -> __flash1 (extended program memory bank1, 64-128KB), ..., addrspace 6 -> __flash5 (extended program memory bank 5, 320KB-384KB). And all other addrspaces fall back to SRAM.

2. For MCU with neither FeatureLPM nor FeatureELPM, all input address spaces fall back to SRAM, and no LPM/ELPM instruction will be generated.

3. For MCU with only FeatureLPM but not FeatureELPM, all access to address space 2-6 fall back to ordinary program space (0-64KB), and only LPM will be generated, and no ELPM will be generated.

[aykevl]

I wonder whether we should generate an error instead of falling back to other memory areas? That would be more consistent with avr-gcc and I think that would also be more correct.

llvm/test/CodeGen/AVR/sections.ll

32

Why not check for what it should be, instead of what it should not be? Something like this (untested):

; CHECK8515: .section .data,"a"

[benshi001]

In D115987#3218125, @aykevl wrote:

I wonder whether we should generate an error instead of falling back to other memory areas? That would be more consistent with avr-gcc and I think that would also be more correct.

Actually an error will be reported by clang as in https://reviews.llvm.org/D115982, which is indeed consistent with avr-gcc.

And in current patch, a brief error will be reported too (though the location information needs to be filled by future patches).

[aykevl]

Looks good to me! A few suggestions below (which aren't necessary, just ideas).

llvm/lib/Target/AVR/AVR.h

49

Suggestion: you could also call this NumAddrSpaces because it contains the number of address spaces.

llvm/lib/Target/AVR/AVRTargetObjectFile.cpp

45–62

To be honest, I don't think this error checking is needed. If there is a section in address space 3 for example, but this is compiled for an atmega328p (which only supports address space 0 and 1), then the necessary ELPM instructions will already cause a compiler error. Having .progmem2.data that the program can't access doesn't harm.

But it doesn't harm either, so feel free to leave it in.

[benshi001]

llvm/lib/Target/AVR/AVR.h

49

I will change it to NumAddrSpaces while commiting.

llvm/lib/Target/AVR/AVR.h

 void initializeAVRShiftExpandPass(PassRegistry &);
 void initializeAVRExpandPseudoPass(PassRegistry &);
 void initializeAVRRelaxMemPass(PassRegistry &);
 
 /// Contains the AVR backend.
 namespace AVR {
 
 /// An integer that identifies all of the supported AVR address spaces.
-enum AddressSpace { DataMemory, ProgramMemory };
+enum AddressSpace {
+  DataMemory,
+  ProgramMemory,
+  ProgramMemory1,
+  ProgramMemory2,
+  ProgramMemory3,
+  ProgramMemory4,
+  ProgramMemory5,
+  NumAddrSpaces,
+};
 
 /// Checks if a given type is a pointer to program memory.
 template <typename T> bool isProgramMemoryAddress(T *V) {
-  return cast<PointerType>(V->getType())->getAddressSpace() == ProgramMemory;
+  auto *PT = cast<PointerType>(V->getType());
+  assert(PT != nullptr && "unexpected MemSDNode");
+  return PT->getAddressSpace() == ProgramMemory ||
+         PT->getAddressSpace() == ProgramMemory1 ||
+         PT->getAddressSpace() == ProgramMemory2 ||
+         PT->getAddressSpace() == ProgramMemory3 ||
+         PT->getAddressSpace() == ProgramMemory4 ||
+         PT->getAddressSpace() == ProgramMemory5;
 }
 
-inline bool isProgramMemoryAccess(MemSDNode const *N) {
-  auto V = N->getMemOperand()->getValue();
+template <typename T> AddressSpace getAddressSpace(T *V) {
+  auto *PT = cast<PointerType>(V->getType());
+  assert(PT != nullptr && "unexpected MemSDNode");
+  unsigned AS = PT->getAddressSpace();
+  if (AS < NumAddrSpaces)
+    return static_cast<AddressSpace>(AS);
+  return NumAddrSpaces;
+}
 
-  return (V != nullptr) ? isProgramMemoryAddress(V) : false;
+inline bool isProgramMemoryAccess(MemSDNode const *N) {
+  auto *V = N->getMemOperand()->getValue();
+  if (V != nullptr && isProgramMemoryAddress(V))
+    return true;
+  return false;
 }
 
 } // end of namespace AVR
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_H

llvm/lib/Target/AVR/AVRTargetObjectFile.h

 public:
   void Initialize(MCContext &ctx, const TargetMachine &TM) override;
 
   MCSection *SelectSectionForGlobal(const GlobalObject *GO, SectionKind Kind,
                                     const TargetMachine &TM) const override;
 
 private:
   MCSection *ProgmemDataSection;
+  MCSection *Progmem1DataSection;
+  MCSection *Progmem2DataSection;
+  MCSection *Progmem3DataSection;
+  MCSection *Progmem4DataSection;
+  MCSection *Progmem5DataSection;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_AVR_TARGET_OBJECT_FILE_H

llvm/lib/Target/AVR/AVRTargetObjectFile.cpp

 //===-- AVRTargetObjectFile.cpp - AVR Object Files ------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "AVRTargetObjectFile.h"
+#include "AVRTargetMachine.h"
 
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCSectionELF.h"
 
 #include "AVR.h"
 
 namespace llvm {
 void AVRTargetObjectFile::Initialize(MCContext &Ctx, const TargetMachine &TM) {
   Base::Initialize(Ctx, TM);
   ProgmemDataSection =
       Ctx.getELFSection(".progmem.data", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+  Progmem1DataSection =
+      Ctx.getELFSection(".progmem1.data", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+  Progmem2DataSection =
+      Ctx.getELFSection(".progmem2.data", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+  Progmem3DataSection =
+      Ctx.getELFSection(".progmem3.data", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+  Progmem4DataSection =
+      Ctx.getELFSection(".progmem4.data", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+  Progmem5DataSection =
+      Ctx.getELFSection(".progmem5.data", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
 }
 
 MCSection *AVRTargetObjectFile::SelectSectionForGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
-  // Global values in flash memory are placed in the progmem.data section
+  // Global values in flash memory are placed in the progmem*.data section
   // unless they already have a user assigned section.
-  if (AVR::isProgramMemoryAddress(GO) && !GO->hasSection() && Kind.isReadOnly())
+  const auto &AVRTM = static_cast<const AVRTargetMachine &>(TM);
+  if (AVR::isProgramMemoryAddress(GO) && !GO->hasSection() &&
+      Kind.isReadOnly()) {
+    // The AVR subtarget should support LPM to access section '.progmem*.data'.
+    if (!AVRTM.getSubtargetImpl()->hasLPM()) {
+      // TODO: Get the global object's location in source file.
+      getContext().reportError(
+          SMLoc(),
+          "Current AVR subtarget does not support accessing program memory");
+      return Base::SelectSectionForGlobal(GO, Kind, TM);
+    }
+    // The AVR subtarget should support ELPM to access section
+    // '.progmem[1|2|3|4|5].data'.
+    if (!AVRTM.getSubtargetImpl()->hasELPM() &&
+        AVR::getAddressSpace(GO) != AVR::ProgramMemory) {
+      // TODO: Get the global object's location in source file.
+      getContext().reportError(SMLoc(),
+                               "Current AVR subtarget does not support "
+                               "accessing extended program memory");
+      return ProgmemDataSection;
+    }
+    switch (AVR::getAddressSpace(GO)) {
+    case AVR::ProgramMemory: // address space 1
       return ProgmemDataSection;
+    case AVR::ProgramMemory1: // address space 2
+      return Progmem1DataSection;
+    case AVR::ProgramMemory2: // address space 3
+      return Progmem2DataSection;
+    case AVR::ProgramMemory3: // address space 4
+      return Progmem3DataSection;
+    case AVR::ProgramMemory4: // address space 5
+      return Progmem4DataSection;
+    case AVR::ProgramMemory5: // address space 6
+      return Progmem5DataSection;
+    default:
+      llvm_unreachable("unexpected program memory index");
+    }
+  }
 
   // Otherwise, we work the same way as ELF.
   return Base::SelectSectionForGlobal(GO, Kind, TM);
 }
 } // end of namespace llvm

llvm/test/CodeGen/AVR/sections.ll

-; RUN: llc < %s -march=avr | FileCheck --check-prefixes=CHECK,NOSECTIONS %s
-; RUN: llc -function-sections -data-sections < %s -march=avr | FileCheck --check-prefixes=CHECK,SECTIONS %s
+; RUN: llc < %s -march=avr --mcpu=atxmega384d3 \
+; RUN:     | FileCheck --check-prefixes=CHECK,NOSECTIONS %s
+; RUN: llc -function-sections -data-sections < %s -march=avr --mcpu=atxmega384d3 \
+; RUN:     | FileCheck --check-prefixes=CHECK,SECTIONS %s
+; RUN: not llc -function-sections -data-sections < %s -march=avr --mcpu=at90s8515 2>&1 \
+; RUN:     | FileCheck --check-prefixes=CHECK-8515 %s
+; RUN: not llc -function-sections -data-sections < %s -march=avr --mcpu=attiny40 2>&1 \
+; RUN:     | FileCheck --check-prefixes=CHECK-tiny40 %s
 
 ; Test that functions (in address space 1) are not considered .progmem data.
 
 ; CHECK: .text
 ; SECTIONS: .text.somefunc,"ax",@progbits
 ; CHECK-LABEL: somefunc:
 define void @somefunc() addrspace(1) {
   ret void
 }
 
 
 ; Test whether global variables are placed in the correct section.
 
 ; Note: avr-gcc would place this global in .progmem.data.flash with
 ; -fdata-sections. The AVR backend does not yet respect -fdata-sections in this
 ; case.
+
 ; CHECK: .section .progmem.data,"a",@progbits
 ; CHECK-LABEL: flash:
 @flash = addrspace(1) constant i16 3
 
+; CHECK: .section .progmem1.data,"a",@progbits
+; CHECK-LABEL: flash1:
+; CHECK-8515: error: Current AVR subtarget does not support accessing extended program memory
+; CHECK-tiny40: error: Current AVR subtarget does not support accessing program memory
+@flash1 = addrspace(2) constant i16 4
+
+; CHECK: .section .progmem2.data,"a",@progbits
+; CHECK-LABEL: flash2:
+; CHECK-8515: error: Current AVR subtarget does not support accessing extended program memory
+; CHECK-tiny40: error: Current AVR subtarget does not support accessing program memory
+@flash2 = addrspace(3) constant i16 5
+
+; CHECK: .section .progmem3.data,"a",@progbits
+; CHECK-LABEL: flash3:
+; CHECK-8515: error: Current AVR subtarget does not support accessing extended program memory
+; CHECK-tiny40: error: Current AVR subtarget does not support accessing program memory
+@flash3 = addrspace(4) constant i16 6
+
+; CHECK: .section .progmem4.data,"a",@progbits
+; CHECK-LABEL: flash4:
+; CHECK-8515: error: Current AVR subtarget does not support accessing extended program memory
+; CHECK-tiny40: error: Current AVR subtarget does not support accessing program memory
+@flash4 = addrspace(5) constant i16 7
+
+; CHECK: .section .progmem5.data,"a",@progbits
+; CHECK-LABEL: flash5:
+; CHECK-8515: error: Current AVR subtarget does not support accessing extended program memory
+; CHECK-tiny40: error: Current AVR subtarget does not support accessing program memory
+@flash5 = addrspace(6) constant i16 8
+
 ; NOSECTIONS: .section .rodata,"a",@progbits
 ; SECTIONS:   .section .rodata.ram1,"a",@progbits
 ; CHECK-LABEL: ram1:
 @ram1 = constant i16 3
 
 ; NOSECTIONS: .data
 ; SECTIONS:   .section .data.ram2,"aw",@progbits
 ; CHECK-LABEL: ram2:
 @ram2 = global i16 3