[PATCH] D96853: [clang][AVR] Support variable decorator '__flash'

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:159
+def err_verify_nonconst_addrspace : Error<
+  "qualifier 'const' is needed for variables in address space '%0'">;
 
----------------
Ok, do you plan to pass anything other than `__flash` here? If not then you don't need to pass an argument in this diagnostic.


================
Comment at: clang/test/CodeGen/avr-flash.c:5
+  static __flash int b[] = {4, 6, 1, 2, 5}; // expected-error {{qualifier 'const' is needed for variables in address space '__flash'}}
+  return b[j] + b[i];
+}
----------------
Is there anything you are testing with these array subscript expressions? If not I suggest removing them and only keep what is needed for testing the new functionality.


================
Comment at: clang/test/CodeGen/avr-flash.c:1
+// RUN: %clang_cc1 -triple avr -emit-llvm-only -verify %s
+
----------------
benshi001 wrote:
> benshi001 wrote:
> > Anastasia wrote:
> > > If you are only checking the diagnostics you should pass:
> > > 
> > > `-emit-llvm-only` -> `-syntax-only`
> > > 
> > > and also it should be moved to `clang/test/Sema`.
> > This test can not run with `-syntax-only`. Since the check is performed in the codegen stage.
> > 
> > I would like to do the check in the Sema stage, but there is no target specific code in the sema stage. And such checks are better to be put into AVRTargetCodeGenInfo.
> I haved moved all `__flash` related to the CodeGen part, now it has nothing to do with `Sema/-syntax-only`.
> I would like to do the check in the Sema stage, but there is no target specific code in the sema stage. And such checks are better to be put into AVRTargetCodeGenInfo

Sema has target hooks and access to the target specific address spaces so you could indeed move them to Sema. Normally we do diagnostics as early as possible but there is no strict rule and there are some tradeoffs. So, you can also keep it here to allow better code incapsulation and simplify maintenance.

If you need to implement the pointer conversion logic you might move this completely to `Sema`. Btw just to highlight the following change https://reviews.llvm.org/D62574 that generalizes some logic that might be useful for your pointer conversions.


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

https://reviews.llvm.org/D96853