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