[PATCH] D36208: [mips] Enable target-specific attributes for MIPS64

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D36208#828881, @atanasyan wrote:

> In https://reviews.llvm.org/D36208#828853, @sdardis wrote:
>
> > Currently there is no support in the backend for the interrupt attribute for mips64 / using N32 & N64 abis, it will give a fatal error. Previously the backend lacked support for the static relocation model which is an expected requirement for interrupt handlers.
> >
> > microMIPS32r(3|6) is only supported for the O32 ABI, as microMIPS64R6 is deprecated and going to be removed. There is no support for the published microMIPS64R3 ISA.
>
>
> I see. What is the better solution to handle unsupported attributes: a) silently ignore them; b) show a warning; c) show an error?


If the attribute is semantically critical, an error is reasonable. Otherwise, we typically warn the user under the ignored attributes group and then drop the attribute from the AST.



================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:263
+def err_attribute_supported_by_o32: Error<
+  "%0 attribute is supported by o32 ABI only">;
 def warn_arm_interrupt_calling_convention : Warning<
----------------
This diagnostic could be more helpful by telling the user how to enable the o32 ABI.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5979-5981
+    if (S.Context.getTargetInfo().getABI() != "o32")
+      S.Diag(Attr.getLoc(), diag::err_attribute_supported_by_o32)
+          << Attr.getName() << D->getLocation();
----------------
Please do not put logic into the switch statement, but instead sink it into a helper function. We hope to remove the boilerplate switch someday, so keeping with the `handleFooAttr()` pattern is preferred. Same below.

Further, why is this not handled by a TargetSpecificAttr in Attr.td? See `TargetMicrosoftCXXABI` and `MSNoVTable` for an example.


Repository:
  rL LLVM

https://reviews.llvm.org/D36208