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

Thu Nov 4 07:31:54 PDT 2021

edward-jones added a comment.

In D109372#3099969 <https://reviews.llvm.org/D109372#3099969>, @jrtc27 wrote:

> In D109372#3099947 <https://reviews.llvm.org/D109372#3099947>, @edward-jones wrote:
>
>> I reverted some of the previous changes I made so that this patch matches the spec as currently written - this means it's two attributes again, and the diagnostic messages have been updated a bit too. The two Clang attributes match to the same LLVM attribute internally though.
>>
>> This is at a stage where more review would be nice. Obviously this is gated on patches to other toolchain components, but I hope that these changes won't change too much now unless the spec also changes.
>
> The whole point of putting it up for review is so you get feedback about the entire direction of the spec, which was written by people who are not experts when it comes to toolchains. You’re supposed to take our feedback, relay it to them and get the draft spec revised. Otherwise, if the spec written by people who don’t know what makes sense for toolchains is regarded as holy and immutable then I’m just going to NACK this as poorly designed and something LLVM shouldn’t bow to implementing, and you’ve wasted my time asking for a full review.

Understood, and apologies for requesting a review without first getting the spec clarified. I had relayed feedback about naming of relocs/attributes and 1 vs 2 attributes offline, but that hadn't reached a conclusion so I had reverted to closer to the spec in the meantime. I've opened issues on the riscv-overlay repository to incorporate the feedback here.

https://github.com/riscv-software-src/riscv-overlay/issues/28
https://github.com/riscv-software-src/riscv-overlay/issues/29
https://github.com/riscv-software-src/riscv-overlay/issues/30

================
Comment at: clang/include/clang/Basic/Attr.td:1797
+def RISCVOverlayCall : InheritableAttr {
+  let Spellings = [GCC<"overlaycall">];
+  let Subjects = SubjectList<[Function]>;
----------------
aaron.ballman wrote:
> 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.
I'll push for an underscore in the names if we're going to have two attributes.

I'm also wondering now whether to use `overlay_function` instead of `overlay_call` since the attribute is marking that the function exists in an overlay rather than the mechanism by which it is called.

================
Comment at: clang/include/clang/Basic/Attr.td:1798
+  let Spellings = [GCC<"overlaycall">];
+  let Subjects = SubjectList<[Function]>;
+  let LangOpts = [RISCVOverlayFunctions];
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > What about Objective-C methods?
> And still wondering about ObjC methods for `RISCVOverlayCallAttr`.
Would simply disallowing this attribute to be used with ObjC be acceptable whilst getting clarification on whether overlays should be usable from ObjC?

================
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
----------------
aaron.ballman wrote:
> 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?
Would it be sufficient to document based on the restrictions defined [[ https://github.com/riscv-software-src/riscv-overlay/blob/master/docs/overlay-hld.adoc#2716-constraints | here ]]? As far as I'm aware LLVM documentation doesn't often refer out to external documents so I assume the attribute's documentation needs to be a self-contained summary?

I'm still not sure about one vs two attribute. One attribute is overall a bit simpler, but any diagnostics are going to need to differentiate anyway. Also, I wonder whether a single `overlay` attribute could confuse users if it was applied to a global function pointer - a naive user might think that the attribute applies to the type of the function rather than the Decl of the pointer.

================
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;
+  }
----------------
aaron.ballman wrote:
> 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.
Yes, the reason for the custom diagnostic is because the diagnostic explicitly requests the user enables `-moverlay`. If I didn't use the custom diagnostic then I could use the generic 'attribute ignored' warning with an additional diagnostic for 'note: use -moverlay to enable __attribute__((overlaycall))', but if the generic warning is emitted from generic code I don't know how I would attach the 'note' diagnostic to it.

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

https://reviews.llvm.org/D109372