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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

There are still unhandled comments in the patch, btw.



================
Comment at: clang/include/clang/Basic/Attr.td:1797
+def RISCVOverlayCall : InheritableAttr {
+  let Spellings = [GCC<"overlaycall">];
+  let Subjects = SubjectList<[Function]>;
----------------
aaron.ballman wrote:
> 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`?
Still wondering about the naming choice.


================
Comment at: clang/include/clang/Basic/Attr.td:1798
+  let Spellings = [GCC<"overlaycall">];
+  let Subjects = SubjectList<[Function]>;
+  let LangOpts = [RISCVOverlayFunctions];
----------------
aaron.ballman wrote:
> What about Objective-C methods?
And still wondering about ObjC methods for `RISCVOverlayCallAttr`.


================
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
----------------
edward-jones wrote:
> jrtc27 wrote:
> > Why not just a single attribute that DTRT based on the type?
> Good point. I'll see if I can do that. The fact we used multiple attributes is mainly a consequence of how we put this together rather than any inherent technical need I think.
I'm confused as to why this is two attributes again. I think using a single attribute makes more sense if they're both documented to be exactly the same thing.

Also, this is missing information about the data overlay restrictions (that it be a global constant variable), and examples. Further, these docs aren't really telling me why I'd use this attribute. Are there other docs we should be linking to that describe what an overlay is, etc?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1883-1885
+  // Overlay functions must have a minimum 4-byte alignment.
+  if (F->getAlignment() < 4 && D->hasAttr<RISCVOverlayCallAttr>())
+    F->setAlignment(llvm::Align(4));
----------------
We should probably document that this attribute changes the function alignment.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:2043-2047
+  if (!S.getLangOpts().Overlay) {
+    S.Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored_overlay)
+        << AL;
+    return;
+  }
----------------
This should be a `LangOpt` in Attr.td and not use a custom diagnostic. However, there is an interesting feature idea hiding in here for a follow-up patch, which is to generate a better diagnostic about which lang opt is required to enable the attribute.


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

https://reviews.llvm.org/D109372