[PATCH] D109372: [RISCV][RFC] Add Clang support for RISC-V overlay system

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1793-1795
+// This is not marked as a TargetSpecificAttr because that would trigger
+// an 'attribute ignored' warning, but we want to check it explicitly and
+// trigger an error.
----------------
This is not typical attribute behavior -- if the target architecture doesn't support the attribute, are the semantics such that an error is appropriate/required? Put another way, why do we want to trigger an error for this attribute while not triggering errors for other target-specific attributes (like calling conventions)?


================
Comment at: clang/include/clang/Basic/Attr.td:1797
+def RISCVOverlayCall : InheritableAttr {
+  let Spellings = [GCC<"overlaycall">];
+  let Subjects = SubjectList<[Function]>;
----------------
Does GCC support this attribute? If not, this should be using the `Clang` spelling (same below).

Also, `overlaycall` is pretty hard to read; why not `overlay_call` and `overlay_data`?


================
Comment at: clang/include/clang/Basic/Attr.td:1798
+  let Spellings = [GCC<"overlaycall">];
+  let Subjects = SubjectList<[Function]>;
+  let LangOpts = [RISCVOverlayFunctions];
----------------
What about Objective-C methods?


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:36
+def err_drv_invalid_riscv_abi_fcomrv : Error<
+  "invalid ABI '%0' when using -fcomrv">;
 def warn_drv_avr_mcu_not_specified : Warning<
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:314
+  "RISC-V 'overlaycall' attribute requires support to be enabled with "
+  "the -fcomrv option">;
+def err_overlaycall_static : Error<
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:321
+  "RISC-V 'overlaydata' attribute requires support to be enabled with "
+  "the -fcomrv option">;
 def warn_unused_parameter : Warning<"unused parameter %0">,
----------------



================
Comment at: clang/lib/Driver/ToolChains/Arch/RISCV.cpp:496
+  // Handle features corresponding to custom language feature
+  // "RISCVOverlayFunctions"
+  if (Args.hasArg(options::OPT_fcomrv)) {
----------------



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2100
+  // If comrv mode is requested, pass on this flag, and produce an error if an
+  // invalid ABI has been requested
+  if (Args.getLastArg(options::OPT_fcomrv)) {
----------------



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4066
 
+  // RISC-V overlay functions support
+  Opts.RISCVOverlayFunctions = Args.hasArg(OPT_fcomrv);
----------------



================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4461
 
+  // RISC-V overlay functions support
+  LangOpts.RISCVOverlayFunctions = Args.hasArg(OPT_fcomrv);
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:3365
+    Diag(New->getLocation(), diag::err_overlaycall_mismatch)
+      << New << OldIsOverlayCall;
+    notePreviousDefinition(Old, New->getLocation());
----------------
You should fix this formatting nit.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2043-2047
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+        << AL << ExpectedFunctionOrMethod;
+    return;
+  }
----------------
This shouldn't be necessary, the automated checking should handle it automatically.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2056-2058
+  // overlaycall implies noinline
+  Attr *A = ::new(S.Context) NoInlineAttr(S.Context, AL);
+  A->setImplicit(true);
----------------
Please use `NoInlineAttr::CreateImplicit()` instead.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8512-8517
+    if (!S.getLangOpts().RISCVOverlayFunctions) {
+      AL.setInvalid();
+      S.Diag(AL.getLoc(), diag::err_overlaydata_unsupported);
+      break;
+    }
+    D->addAttr(::new (S.Context) RISCVOverlayDataAttr(S.Context, AL));
----------------
Please write a `handle` function for this as with all the other attributes (we have hopes to someday do more tablegen in this area, so sticking with the same pattern in this `switch` is strongly preferred).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109372