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