[PATCH] D36208: [mips] Enable `long_call/short_call` attributes on MIPS64

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:268
 def TargetMips : TargetArch<["mips", "mipsel"]>;
+def TargetAnyMips : TargetArch<["mips", "mipsel", "mips64", "mips64el"]>;
 def TargetMSP430 : TargetArch<["msp430"]>;
----------------
Can you also rename `TargetMips` to something that distinguishes it from `TargetAnyMips`, like we have with `TargetX86` vs `TargetAnyX86`? I'd be hard-pressed to notice the difference between these two target names when reviewing an attribute, but I'm not familiar enough with the MIPS architecture to have a particularly good suggestion for improvement.


================
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();
----------------
atanasyan wrote:
> aaron.ballman wrote:
> > 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.
> > Further, why is this not handled by a TargetSpecificAttr in Attr.td? See TargetMicrosoftCXXABI and MSNoVTable for an example.
> 
> I thought about this, but in that case compiler will show "unknown attribute 'mips16' ignored" warning in case of using `mips16` on MIPS64 targets. As to me this warning is not quite correct in that case because `mips16`, `micromips`, `interrupt` all are known MIPS-specific attributes, but not applicable on MIPS64 by various reasons.
> As to me this warning is not quite correct in that case because mips16, micromips, interrupt all are known MIPS-specific attributes, but not applicable on MIPS64 by various reasons.

The same is true of all target-specific attributes -- they're known to the compiler, but their usage is not known on that specific target architecture. I would not be opposed to a patch that distinguished between the two situations, however (completely unknown attribute/and attribute that's known to be prohibited).


Repository:
  rL LLVM

https://reviews.llvm.org/D36208