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

[Ayke via Phabricator via llvm-commits]

aykevl accepted this revision.
aykevl added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jacquesguan.

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



================
Comment at: llvm/lib/Target/AVR/AVR.h:50
+  ProgramMemory5,
+  InvalidAddrSpace,
+};
----------------
Suggestion: you could also call this `NumAddrSpaces` because it contains the number of address spaces.


================
Comment at: llvm/lib/Target/AVR/AVRTargetObjectFile.cpp:45-62
+    // 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);
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115987/new/

https://reviews.llvm.org/D115987