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