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

[Edward Jones via Phabricator via cfe-commits]

edward-jones added a comment.

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

> In D109372#2987405 <https://reviews.llvm.org/D109372#2987405>, @MaskRay wrote:
>
>> The name "overlay" is ambiguous. Even after I ruled out Gentoo Overlay and overlayfs, I am thinking whether this has anything to do with `OVERLAY` description in a linker script: https://sourceware.org/binutils/docs/ld/Overlay-Description.html#Overlay-Description
>>
>>> which are used to mark functions or global data as only accessible through the overlay engine
>>
>> Can you give more descriptions for folks who don't follow the RISC-V side proposal but need to review your changes? :)
>
> Basically hardware-assisted code+rodata banking (I guess either by actually banking ROMs or just paging stuff in and out) that's mostly transparent to software. Functions at the boundary of components (don't know what the granularity is) use a weird indirect calling convention where you instead call into some magic runtime with a unique ID for the callee, it ensures everything's loaded and then tail calls it for you.

Yes it's essentially this. The start of the proposal for this 'overlay' system for RISC-V is a FOSDEM talk 'Cacheable Overlay Manager RISC‐V' <https://archive.fosdem.org/2020/schedule/event/riscv_comrv/attachments/paper/3441/export/events/attachments/riscv_comrv/paper/3441/Summit_Paper_cacheable_overlay_manager_FOSDEM.pdf>. That's also the source of the weird name for the `-fcomrv` option name.

Would something like `-foverlay-manager` make more sense? (maybe an `-m` option would actually be more appropriate given this is still very RISC-V specific?). I'm not sure how to disambiguate from the many overloaded meanings of 'overlay'.

Thank's for the feedback. I'll update this and come back with a tidier patch.



================
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.
----------------
aaron.ballman wrote:
> 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)?
If the attribute is being used then the expectation is that it is being supported by an overlay engine, and if that isn't the case then it implies an error in the build setup. That said I need to check this because I'm not convinced it's necessary.


================
Comment at: clang/include/clang/Basic/Attr.td:1796
+// trigger an error.
+def RISCVOverlayCall : InheritableAttr {
+  let Spellings = [GCC<"overlaycall">];
----------------
jrtc27 wrote:
> 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).
I don't think much consideration has been given to other compilers, but would it be unreasonable for the interface to this feature to not necessarily be identical between GNU and non-GNU compilers?

That said, I'm happy to switch to a keyword, especially if as you mention there are cases where an attribute can silently go missing without error. I'm not sure on the distinction of when to use a keyword vs attribute though, given that keywords are used pretty sparingly in comparison.


================
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
----------------
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.


================
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}}
+};
----------------
jrtc27 wrote:
> This error message is misleading. The semantics also don't seem great to me.
>From recollection the current overlay system only works with externally visible symbols and these semantics are a consequence of that.

That said, it's not documented in the code, and as you point out the error message is just wrong so I'll fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109372