[PATCH] D115982: [clang][AVR] Implement '__flashN' for variables on different flash banks

[Ayke via Phabricator via cfe-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!



================
Comment at: clang/lib/Basic/Targets/AVR.cpp:27
   const char *DefineName;
+  const int MaxFlashBank; // -1 means the device does not support LPM/ELPM.
 };
----------------
This works and is fine, but I think you could also name it `NumFlashBanks` so that it is the number of flash banks supported on the device (0 if LPM/ELPM is not supported). With 0 for the attiny11 (which has no accessible flash banks) and 1 for the attiny22 (because it has only one flash bank that can be accessed: flash bank 0).

Just a suggestion, the current system looks good to me.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:8290
+    if (isTargetAddressSpace(AS) && 1 <= toTargetAddressSpace(AS) &&
+        toTargetAddressSpace(AS) <= 6 && !D->getType().isConstQualified())
       CGM.getDiags().Report(D->getLocation(),
----------------
These hardcoded numbers are a bit unfortunate, but OK.


================
Comment at: clang/test/Sema/avr-flash.c:10
+  static __flash5 const int a5[] = {4, 6}; // expected-error {{unknown type name '__flash5'}}
+  // TODO: It would be better to report "'__flash5' is not supported on at908515".
+  return a0[n] + a1[n];
----------------
I think this can be implemented by always setting the `__flash*` defines and checking whether they are valid for the current device in `getGlobalVarAddressSpace`.


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

https://reviews.llvm.org/D115982