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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
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.
----------------
edward-jones wrote:
> 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.
Thanks for looking into it! My preference is for the unknown attribute to be ignored (with diagnostic) on other targets unless there's a strong argument for why it's an error.


================
Comment at: clang/include/clang/Basic/Attr.td:1796
+// trigger an error.
+def RISCVOverlayCall : InheritableAttr {
+  let Spellings = [GCC<"overlaycall">];
----------------
edward-jones wrote:
> 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.
This isn't a type attribute, so I don't think there's a risk of the attribute getting lost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109372