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

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added a comment.

I don't see anything giving an error if you try and use the new badly-named option for an architecture other than RISC-V (beyond the usual unused argument warning that's only an error with -Werror)?



================
Comment at: clang/include/clang/Basic/Attr.td:348
+// Language option for Overlay functions
+def RISCVOverlayFunctions : LangOpt<"RISCVOverlayFunctions">;
+
----------------
I thought the idea for this proposal was to come up with something that wasn't RISC-V specific (an architecture-independent specification and a RISC-V-specific supplement for how that is mapped to RISC-V) to get industry buy-in?


================
Comment at: clang/include/clang/Basic/Attr.td:1796
+// trigger an error.
+def RISCVOverlayCall : InheritableAttr {
+  let Spellings = [GCC<"overlaycall">];
----------------
If you want this to be portable to non-GNU compilers you should consider using a keyword instead (which can still map to an attribute internally). That also tends to get you better errors (there are places where type attributes can get silently ignored currently).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2149
+  let Content = [{
+``__attribute__((overlaycall))`` indicates that a function resides in an
+overlay and therefore any calls to or from that function must be handled
----------------
Why not just a single attribute that DTRT based on the type?


================
Comment at: clang/include/clang/Driver/Options.td:3232
   HelpText<"Enable use of experimental RISC-V extensions.">;
+def fcomrv : Flag<["-"], "fcomrv">, Group<f_Group>,
+  Flags<[CC1Option]>,
----------------
What on earth is this name?


================
Comment at: clang/test/CodeGen/riscv-overlaycall.c:1
+// RUN: %clang_cc1 %s -triple=riscv64 -fcomrv -emit-llvm -o - \
+// RUN:    | FileCheck %s
----------------
Use update_cc_test_checks.py


================
Comment at: clang/test/Driver/riscv-comrv.c:3
+//
+// REQUIRES: riscv-registered-target
+//
----------------
Don't write a driver test that requires the backend, you don't need that to test this


================
Comment at: clang/test/Sema/riscv-overlaycall-namespace.cpp:7
+public:
+  static int X() __attribute__((overlaycall)) { return 0; } // expected-error {{RISC-V 'overlaycall' attribute not supported on static functions}}
+};
----------------
This error message is misleading. The semantics also don't seem great to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109372