[mips] Enable `long_call/short_call` attributes on MIPS64

[atanasyan]

Summary

This change enables long_call/short_call/far/near attributes on MIPS64 targets.

[sdardis]

Some comments:

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.

[atanasyan]

In 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?

[aaron.ballman]

In D36208#828881, @atanasyan wrote:

In 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.

include/clang/Basic/DiagnosticSemaKinds.td

263 ↗

(On Diff #109272)

This diagnostic could be more helpful by telling the user how to enable the o32 ABI.

lib/Sema/SemaDeclAttr.cpp

5979–5981 ↗

(On Diff #109272)

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.

[sdardis]

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

For the micromips attribute, I believe it should be an error. My rational there is that the user has requested something that the compiler cannot perform.

[aaron.ballman]

In D36208#828955, @sdardis wrote:

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

For the interrupt attribute, I think it should behave the same as the other target-specific interrupt attributes unless there's a very good reason to be inconsistent.

For the micromips attribute, I believe it should be an error. My rational there is that the user has requested something that the compiler cannot perform.

That's not really sufficient rationale, because the same logic can be said of any ignored attribute. As best I can tell from the docs on this attribute, this controls code generation to turn on or off micromips code generation, which sounds like the code otherwise generated may still work just fine. e.g., void f(void) __attribute__((micromips)) {} -- if you compile that code on a non-MIPS target, why should it produce an error instead of merely warning the user that the attribute is ignored?

[sdardis]

@aaron.ballman I missed your first comments when I'd submitted mine.

In D36208#828957, @aaron.ballman wrote:

In D36208#828955, @sdardis wrote:

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

For the interrupt attribute, I think it should behave the same as the other target-specific interrupt attributes unless there's a very good reason to be inconsistent.

My primary thought there was that the backend will error out with "LLVM ERROR: "interrupt" attribute is only supported for the O32 ABI on MIPS32R2+ at the present time." if the attribute is seen anywhere by the backend for MIPS64. Having clang detect this case and being able to point out where in the code being compiled the attribute exists is better than having LLVM declaring there's an error somewhere and giving up. It is semantically critical as interrupt handlers need specific prologue+epilog sequences. I don't object to preserving the current behaviour of warning + ignoring it though.

For the micromips attribute, I believe it should be an error. My rational there is that the user has requested something that the compiler cannot perform.

That's not really sufficient rationale, because the same logic can be said of any ignored attribute. As best I can tell from the docs on this attribute, this controls code generation to turn on or off micromips code generation, which sounds like the code otherwise generated may still work just fine. e.g., void f(void) __attribute__((micromips)) {} -- if you compile that code on a non-MIPS target, why should it produce an error instead of merely warning the user that the attribute is ignored?

I stand corrected, we should just warn + ignore the micromips attribute for MIPS64 then.

[aaron.ballman]

In D36208#829006, @sdardis wrote:

@aaron.ballman I missed your first comments when I'd submitted mine.

In D36208#828957, @aaron.ballman wrote:

In D36208#828955, @sdardis wrote:

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

For the interrupt attribute, I think it should behave the same as the other target-specific interrupt attributes unless there's a very good reason to be inconsistent.

My primary thought there was that the backend will error out with "LLVM ERROR: "interrupt" attribute is only supported for the O32 ABI on MIPS32R2+ at the present time." if the attribute is seen anywhere by the backend for MIPS64.

Having the backend error out is not the best user experience, so I agree that Clang should help prevent that. However, it seems just as likely the issue should be solved in the backend instead.

Having clang detect this case and being able to point out where in the code being compiled the attribute exists is better than having LLVM declaring there's an error somewhere and giving up. It is semantically critical as interrupt handlers need specific prologue+epilog sequences. I don't object to preserving the current behaviour of warning + ignoring it though.

I agree that Clang should definitely let the user know what's going on rather than leaving it to a backend error, but I think the behavior where we warn that the attribute is ignored and drop it is the best approach. I believe that if we gate the attribute on the ABI, that should happen automatically.

[atanasyan]

lib/Sema/SemaDeclAttr.cpp

5979–5981 ↗

(On Diff #109272)

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.

[atanasyan]

Simplify and reduce the patch. Keep mips16, micromips, and interrupt attributes handling unchanged. The only MIPS specific attributes supported on MIPS64 targets are long_call/short_call/far/near.

[aaron.ballman]

include/clang/Basic/Attr.td

268 ↗

(On Diff #109528)

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.

lib/Sema/SemaDeclAttr.cpp

5979–5981 ↗

(On Diff #109272)

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).

[atanasyan]

• Renamed TargetMips to TargetMips32

[aaron.ballman]

LGTM, thanks!

[atanasyan]

Thanks for review.

cfe/trunk/include/clang/Basic/Attr.td

 // enumeration in Triple.h.
 class TargetArch<list<string> arches> {
   list<string> Arches = arches;
   list<string> OSes;
   list<string> CXXABIs;
 }
 def TargetARM : TargetArch<["arm", "thumb", "armeb", "thumbeb"]>;
 def TargetAVR : TargetArch<["avr"]>;
-def TargetMips : TargetArch<["mips", "mipsel"]>;
+def TargetMips32 : TargetArch<["mips", "mipsel"]>;
+def TargetAnyMips : TargetArch<["mips", "mipsel", "mips64", "mips64el"]>;
 def TargetMSP430 : TargetArch<["msp430"]>;
 def TargetX86 : TargetArch<["x86"]>;
 def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
 def TargetWindows : TargetArch<["x86", "x86_64", "arm", "thumb", "aarch64"]> {
   let OSes = ["Win32"];
 }
 def TargetMicrosoftCXXABI : TargetArch<["x86", "x86_64", "arm", "thumb", "aarch64"]> {
   let CXXABIs = ["Microsoft"];
def MSP430Interrupt : InheritableAttr, TargetSpecificAttr<TargetMSP430> {
   // and AnyX86Interrupt's spellings must match.
   let Spellings = [GNU<"interrupt">];
   let Args = [UnsignedArgument<"Number">];
   let ParseKind = "Interrupt";
   let HasCustomParsing = 1;
   let Documentation = [Undocumented];
 }
 
-def Mips16 : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def Mips16 : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"mips16">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
-def MipsInterrupt : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def MipsInterrupt : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   // NOTE: If you add any additional spellings, ARMInterrupt's,
   // MSP430Interrupt's and AnyX86Interrupt's spellings must match.
   let Spellings = [GNU<"interrupt">];
   let Subjects = SubjectList<[Function]>;
   let Args = [EnumArgument<"Interrupt", "InterruptType",
                            ["vector=sw0", "vector=sw1", "vector=hw0",
                             "vector=hw1", "vector=hw2", "vector=hw3",
                             "vector=hw4", "vector=hw5", "eic", ""],
                            ["sw0", "sw1", "hw0", "hw1", "hw2", "hw3",
                             "hw4", "hw5", "eic", "eic"]
                            >];
   let ParseKind = "Interrupt";
   let Documentation = [MipsInterruptDocs];
 }
 
-def MicroMips : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def MicroMips : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"micromips">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [MicroMipsDocs];
 }
 
-def MipsLongCall : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def MipsLongCall : InheritableAttr, TargetSpecificAttr<TargetAnyMips> {
   let Spellings = [GCC<"long_call">, GCC<"far">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [MipsLongCallStyleDocs];
 }
 
-def MipsShortCall : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def MipsShortCall : InheritableAttr, TargetSpecificAttr<TargetAnyMips> {
   let Spellings = [GCC<"short_call">, GCC<"near">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [MipsShortCallStyleDocs];
 }
 
 def Mode : Attr {
   let Spellings = [GCC<"mode">];
   let Subjects = SubjectList<[Var, Enum, TypedefName, Field], ErrorDiag,

 }
 
 def NoInline : InheritableAttr {
   let Spellings = [GCC<"noinline">, Declspec<"noinline">];
   let Subjects = SubjectList<[Function]>;
   let Documentation = [Undocumented];
 }
 
-def NoMips16 : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def NoMips16 : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"nomips16">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [Undocumented];
 }
 
-def NoMicroMips : InheritableAttr, TargetSpecificAttr<TargetMips> {
+def NoMicroMips : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"nomicromips">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [MicroMipsDocs];
 }
 
 // This is not a TargetSpecificAttr so that is silently accepted and
 // ignored on other targets as encouraged by the OpenCL spec.
 //

cfe/trunk/test/CodeGen/long-call-attr.c

 // RUN: %clang_cc1 -triple mips-linux-gnu -emit-llvm  -o  - %s | FileCheck %s
+// RUN: %clang_cc1 -triple mips64-linux-gnu -emit-llvm  -o  - %s | FileCheck %s
 
 void __attribute__((long_call)) foo1 (void);
 void __attribute__((short_call)) foo4 (void);
 
 void __attribute__((far)) foo2 (void) {}
 
 // CHECK: define void @foo2() [[FAR:#[0-9]+]]
 

cfe/trunk/test/Sema/attr-long-call.c

 // RUN: %clang_cc1 -triple mips-linux-gnu -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple mips64-linux-gnu -fsyntax-only -verify %s
 
 __attribute__((long_call(0))) void foo1();  // expected-error {{'long_call' attribute takes no arguments}}
 __attribute__((short_call(0))) void foo9();  // expected-error {{'short_call' attribute takes no arguments}}
 __attribute__((far(0))) void foo2();  // expected-error {{'far' attribute takes no arguments}}
 __attribute__((near(0))) void foo3();  // expected-error {{'near' attribute takes no arguments}}
 
 __attribute((long_call)) int a; // expected-warning {{attribute only applies to functions}}
 __attribute((short_call)) int d; // expected-warning {{attribute only applies to functions}}