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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits