[RISCV] Add support for interrupt attribute

[apazos]

Summary

Clang supports the GNU style `__attribute__((interrupt))` attribute on RISCV targets.
Permissible values for this parameter are user, supervisor, and machine.
If there is no parameter, then it defaults to machine.
Reference: https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Function-Attributes.html
Based on initial patch by Zhaoshi Zheng.

[simoncook]

I think this should also cover mismatched arguments if the attribute appears several times, and reject/warn about the attribute combination in these cases.

For example, __attribute__((interrupt("machine"))) __attribute__((interrupt("user"))) void foo() {} is accepted, and adds the attribute "interrupt"="user". It seems whichever is first used appears on the function.

I don't have an up to date GCC to compare to see what it does in this case, nor am sure whether repeating the same interrupt type should be accepted.

[apazos]

Hi Simon, I have added a warning for repeated interrupt attributes.

[aaron.ballman]

lib/CodeGen/TargetInfo.cpp

8966

You can use const auto * here instead of repeating the type.

lib/Sema/SemaDeclAttr.cpp

5280

Please call checkAttributeNumArgs() instead; the error you're using is incorrect (it's used for variadic parameters where you receive more arguments than you expect).

5301

I don't think you need to perform this check -- I believe it's handled automatically (because you don't have custom parsing enabled).

test/Sema/riscv-interrupt-attr.c

19

You should also add tests for:

__attribute__((interrupt("user"))) void f(void);
__attribute__((interrupt("machine"))) void f(void);

void f(void) { }

[[gnu::interrupt("user") gnu::interrupt("machine")]] void g() {}

[[gnu::interrupt("user")]] [[gnu::interrupt("machine")]] void h() {}

24

Do you intend for functions without a prototype to be accepted? foo8() can be passed an arbitrary number of arguments, which is a bit different than what I thought you wanted the semantic check to be.

27

I'm a bit surprised that this is not an error -- the argument is provided, so I don't know why this should be treated as acceptable.

[apazos]

lib/CodeGen/TargetInfo.cpp

8966

Thanks Aaron, will do the cleanup.

lib/Sema/SemaDeclAttr.cpp

5280

The argument is optional and at most one argument , I will use checkAttributeAtMostNumArgs instead

5301

I think need it. Will double check in the test.

test/Sema/riscv-interrupt-attr.c

19

For this test case tt seems LLVM honors the last setting, "machine".
But gcc is honoring the first.
I think the last setting should prevail. Will check with GCC folks.

27

Good catch, In include/clang/Basic/Attr.td I mapped "" to the default machine mode.
I will change it to align with GCC behavior.

[apazos]

Addressed review comments.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5280

Ah, yeah, I didn't pick up the optional part when writing my comment; this LG.

5301

See handleCommonAttributeFeatures() -- it calls diagnoseAppertainsTo() which handles this for you. As it is, your check here does not match the subject list on the attribute. The declarative bit says it only appertains to a function and this check is for a function or Obj-C method.

Which brings up my next question: should this appertain to an ObjC method?

test/Sema/riscv-interrupt-attr.c

19

Do all of these cases get diagnosed as being a repeated interrupt attribute? Should add them as test cases.

[apazos]

lib/Sema/SemaDeclAttr.cpp

5301

It looks like handleCommonAttributeFeatures should take care of the check, but I do not see it happening, it returns true in AL.diagnoseAppertainsTo(S, D) even when we have
struct a test attribute((interrupt));

I will remove the Subjects in Attr.td and keep the checks as they are in handleRISCVInterruptAttr.

Several other targets do the same thing, they are reusing the helper functions that apply to both Function or Method. We would have to create helper functions just for function types.

test/Sema/riscv-interrupt-attr.c

19

The warning for repeated attribute is when it occurs more than once in the same declaration. If you have repeated declarations, the last one prevails.

[apazos]

Updated tests and removed Subjects from Attr.td

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5301

Ah, the reason is because the parsed attributes that share a ParseKind can have different subject lists, so there's no way to do the semantic checking at that point -- we don't know which semantic attribute to check the subjects against until later.

Please put the Subjects list back in to Attr.td; it's still useful declarative information and I may solve this problem someday in the future.

I am not tied to whether the attribute appertains to a function and an obj-c method as that depends on the attribute in question, but the code as it stands is wrong. It checks whether the declaration is a function or a method and then tells the user the attribute can only appertain to a function and not a method. Which is correct?

5305

I would have assumed this would be: !hasFunctionProto(D) || getFunctionOrMethodNumParams(D) != 0, but it depends on whether you want to support K&R C functions.

test/Sema/riscv-interrupt-attr.c

19

Please document this in AttrDocs.td.

24

This question remains outstanding.

[apazos]

lib/Sema/SemaDeclAttr.cpp

5301

Sure I can add Subjects back in.
I will remove the helper function and use the simple check D->getFunctionType() == nullptr.

5305

hasFunctionProto also returns true for a function definition like this one
attribute((interrupt)) void foo1(int) {}.

test/Sema/riscv-interrupt-attr.c

19

Sure, I can add that info to the description.

24

The checks are validating both function definitions and function prototypes like these:
_attribute((interrupt)) void foo1() {}
attribute__((interrupt)) void foo(void);
Not sure what the confusion is.

[aaron.ballman]

lib/Sema/SemaDeclAttr.cpp

5305

That's correct, which is why I recommended the predicate the way I did. If the function has no prototype, we want to diagnose it. If the function has a prototype, we want to see how many parameters are listed and if there are more than zero parameters, we want to diagnose it.

test/Sema/riscv-interrupt-attr.c

24

Ah, now I see where the confusion is.

In C, an empty parameter list declares a function without a prototype; functions without prototypes can accept any number of arguments. To declare a function that accepts no arguments, you must have a prototype for the function and the parameter list is void. In C++, all functions are prototyped and an empty parameter list is equivalent to a parameter list of void. The word "prototype" doesn't mean "forward declaration". e.g.,

// C code
void foo1(); // Declaration; no prototype; accepts any number of arguments.
void foo2() {} // Definition; no prototype; accepts any number of arguments.
void foo3(void); // Declaration; prototype; accepts no arguments.
void foo4(void) {} // Definition; prototype; accepts no arguments.

foo2(1, 2, 3); // ok
foo4(1, 2, 3); // error

Because a function without a prototype can accept any number of arguments, I think you want to diagnose such a function signature.

[apazos]

test/Sema/riscv-interrupt-attr.c

24

Thanks for clarifying.

I checked GCC behavior and it is less strict. For the example below, it silently accepts the interrupt attribute.

extern int foo2();
attribute((interrupt)) void foo();
void foo() {

foo2();

}

while in LLVM we would be rejecting with the message:
RISC-V 'interrupt' attribute only applies to functions that have no parameters.

I find the reuse of the message confusing.

If we want stricter rule then we probably also need a specific message for the missing prototype.

[aaron.ballman]

test/Sema/riscv-interrupt-attr.c

24

I checked GCC behavior and it is less strict. For the example below, it silently accepts the interrupt attribute.

Does it drop the attribute?

If we want stricter rule then we probably also need a specific message for the missing prototype.

If GCC silently drops the attribute in this case then we definitely want a more strict rule. We already have a good diagnostic for this: warn_attribute_wrong_decl_type with the expected type diagnostic index being ExpectedFunctionWithProtoType.

[apazos]

test/Sema/riscv-interrupt-attr.c

24

It does not drop, it compiles without warnings and it produces the code that is expected when interrupt attribute is set.

[apazos]

Made the check/warning for prototype explicit.

[aaron.ballman]

test/Sema/riscv-interrupt-attr.c

24

Oh! In that case, it's perfectly reasonable for us to support the construct as well. I'm really sorry for the churn this back-and-forth has caused. I just wanted to make sure that the runtime behavior matches GCC and that we diagnose any circumstance under which we're dropping the attribute.

I like the most of the state of this test file where it uses (void) as the parameter list for most of the functions, so how about we keep those changes? I'd leave foo4() without the prototype and just remove the diagnostic I asked you to introduce in the last patch.

[apazos]

Thanks Aaron, I appreciate you taking the time to review.
I have updated the test and removed the warning as discussed.

[aaron.ballman]

LGTM! Do you need me to commit on your behalf, or do you have commit privileges?

[apazos]

Hi Aaron, I have commit rights and will commit if after Alex and other RISC-V colleagues get a chance to review too.

[asb]

This looks good to me with two caveats

• the tests don't seem to check that the "machine" is the default mode when the "interrupt" attribute has no arguments.
• Although the conversion from RISCVInterruptAttr::user to "user" etc is trivial, it would probably be worth testing the emitted attributes are as expected?

[apazos]

• Rebased the patch.
• Udpated test case to check for IR attribute.

[asb]

Looks good to me.

include/clang/Basic/Attr.td

 class TargetArch<list<string> arches> : TargetSpec {
   let Arches = arches;
 }
 def TargetARM : TargetArch<["arm", "thumb", "armeb", "thumbeb"]>;
 def TargetAVR : TargetArch<["avr"]>;
 def TargetMips32 : TargetArch<["mips", "mipsel"]>;
 def TargetAnyMips : TargetArch<["mips", "mipsel", "mips64", "mips64el"]>;
 def TargetMSP430 : TargetArch<["msp430"]>;
+def TargetRISCV : TargetArch<["riscv32", "riscv64"]>;
 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 NoMicroMips : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Spellings = [GCC<"nomicromips">];
   let Subjects = SubjectList<[Function], ErrorDiag>;
   let Documentation = [MicroMipsDocs];
 }
 
+def RISCVInterrupt : InheritableAttr, TargetSpecificAttr<TargetRISCV> {
+  let Spellings = [GCC<"interrupt">];
+  let Subjects = SubjectList<[Function]>;
+  let Args = [EnumArgument<"Interrupt", "InterruptType",
+                           ["user", "supervisor", "machine"],
+                           ["user", "supervisor", "machine"],
+                           1>];
+  let ParseKind = "Interrupt";
+  let Documentation = [RISCVInterruptDocs];
+}
+
 // This is not a TargetSpecificAttr so that is silently accepted and
 // ignored on other targets as encouraged by the OpenCL spec.
 //
 // See OpenCL 1.2 6.11.5: "It is our intention that a particular
 // implementation of OpenCL be free to ignore all attributes and the
 // resulting executable binary will produce the same result."
 //
 // However, only AMD GPU targets will emit the corresponding IR

include/clang/Basic/AttrDocs.td

 
 These attributes have no effect for position-independent code.
 
 These attributes take priority over command line switches such
 as ``-mlong-calls`` and ``-mno-long-calls``.
   }];
 }
 
+def RISCVInterruptDocs : Documentation {
+  let Category = DocCatFunction;
+  let Heading = "interrupt (RISCV)";
+  let Content = [{
+Clang supports the GNU style ``__attribute__((interrupt))`` attribute on RISCV
+targets. This attribute may be attached to a function definition and instructs
+the backend to generate appropriate function entry/exit code so that it can be
+used directly as an interrupt service routine.
+
+Permissible values for this parameter are ``user``, ``supervisor``,
+and ``machine``. If there is no parameter, then it defaults to machine.
+
+Repeated interrupt attribute on the same declaration will cause a warning
+to be emitted. In case of repeated declarations, the last one prevails.
+
+Refer to:
+https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Function-Attributes.html
+https://riscv.org/specifications/privileged-isa/
+The RISC-V Instruction Set Manual Volume II: Privileged Architecture
+Version 1.10.
+  }];
+}
+
 def AVRInterruptDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "interrupt (AVR)";
   let Content = [{
 Clang supports the GNU style ``__attribute__((interrupt))`` attribute on
 AVR targets. This attribute may be attached to a function definition and instructs
 the backend to generate appropriate function entry/exit code so that it can be used
 directly as an interrupt service routine.

include/clang/Basic/DiagnosticSemaKinds.td

def err_anyx86_interrupt_called : Error<
   "interrupt service routine cannot be called directly">;
 def warn_arm_interrupt_calling_convention : Warning<
    "call to function without interrupt attribute could clobber interruptee's VFP registers">,
    InGroup<Extra>;
 def warn_mips_interrupt_attribute : Warning<
    "MIPS 'interrupt' attribute only applies to functions that have "
    "%select{no parameters|a 'void' return type}0">,
    InGroup<IgnoredAttributes>;
+def warn_riscv_repeated_interrupt_attribute : Warning<
+  "repeated RISC-V 'interrupt' attribute">, InGroup<IgnoredAttributes>;
+def note_riscv_repeated_interrupt_attribute : Note<
+  "repeated RISC-V 'interrupt' attribute is here">;
+def warn_riscv_interrupt_attribute : Warning<
+   "RISC-V 'interrupt' attribute only applies to functions that have "
+   "%select{no parameters|a 'void' return type|a prototype}0">,
+   InGroup<IgnoredAttributes>;
 def warn_unused_parameter : Warning<"unused parameter %0">,
   InGroup<UnusedParameter>, DefaultIgnore;
 def warn_unused_variable : Warning<"unused variable %0">,
   InGroup<UnusedVariable>, DefaultIgnore;
 def warn_unused_local_typedef : Warning<
   "unused %select{typedef|type alias}0 %1">,
   InGroup<UnusedLocalTypedef>, DefaultIgnore;
-def warn_unused_property_backing_ivar : 
+def warn_unused_property_backing_ivar :
   Warning<"ivar %0 which backs the property is not "
   "referenced in this property's accessor">,
   InGroup<UnusedPropertyIvar>, DefaultIgnore;
 def warn_unused_const_variable : Warning<"unused variable %0">,
   InGroup<UnusedConstVariable>, DefaultIgnore;
 def warn_unused_exception_param : Warning<"unused exception parameter %0">,
   InGroup<UnusedExceptionParameter>, DefaultIgnore;
 def warn_decl_in_param_list : Warning<

lib/CodeGen/TargetInfo.cpp

ABIArgInfo RISCVABIInfo::extendType(QualType Ty) const {
   return ABIArgInfo::getExtend(Ty);
 }
 
 namespace {
 class RISCVTargetCodeGenInfo : public TargetCodeGenInfo {
 public:
   RISCVTargetCodeGenInfo(CodeGen::CodeGenTypes &CGT, unsigned XLen)
       : TargetCodeGenInfo(new RISCVABIInfo(CGT, XLen)) {}
+
+  void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
+                           CodeGen::CodeGenModule &CGM) const override {
+    const auto *FD = dyn_cast_or_null<FunctionDecl>(D);
+    if (!FD) return;
+
+    const auto *Attr = FD->getAttr<RISCVInterruptAttr>();
+    if (!Attr)
+      return;
+
+    const char *Kind;
+    switch (Attr->getInterrupt()) {
+    case RISCVInterruptAttr::user: Kind = "user"; break;
+    case RISCVInterruptAttr::supervisor: Kind = "supervisor"; break;
+    case RISCVInterruptAttr::machine: Kind = "machine"; break;
+    }
+
+    auto *Fn = cast<llvm::Function>(GV);
+
+    Fn->addFnAttr("interrupt", Kind);
+  }
 };
 } // namespace
 
 //===----------------------------------------------------------------------===//
 // Driver code
 //===----------------------------------------------------------------------===//
 
 bool CodeGenModule::supportsCOMDAT() const {

lib/Sema/SemaDeclAttr.cpp

static void handleAVRSignalAttr(Sema &S, Decl *D, const AttributeList &AL) {
   }
 
   if (!checkAttributeNumArgs(S, AL, 0))
     return;
 
   handleSimpleAttribute<AVRSignalAttr>(S, D, AL);
 }
 
+static void handleRISCVInterruptAttr(Sema &S, Decl *D,
+                                     const AttributeList &AL) {
+  // Warn about repeated attributes.
+  if (const auto *A = D->getAttr<RISCVInterruptAttr>()) {
+    S.Diag(AL.getRange().getBegin(),
+      diag::warn_riscv_repeated_interrupt_attribute);
+    S.Diag(A->getLocation(), diag::note_riscv_repeated_interrupt_attribute);
+    return;
+  }
+
+  // Check the attribute argument. Argument is optional.
+  if (!checkAttributeAtMostNumArgs(S, AL, 1))
+    return;
+
+  StringRef Str;
+  SourceLocation ArgLoc;
+
+  // 'machine'is the default interrupt mode.
+  if (AL.getNumArgs() == 0)
+    Str = "machine";
+  else if (!S.checkStringLiteralArgumentAttr(AL, 0, Str, &ArgLoc))
+    return;
+
+  // Semantic checks for a function with the 'interrupt' attribute:
+  // - Must be a function.
+  // - Must have no parameters.
+  // - Must have the 'void' return type.
+  // - The attribute itself must either have no argument or one of the
+  //   valid interrupt types, see [RISCVInterruptDocs].
+
+  if (D->getFunctionType() == nullptr) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+      << "'interrupt'" << ExpectedFunction;
+    return;
+  }
+
+  if (!hasFunctionProto(D)) {
+    S.Diag(D->getLocation(), diag::warn_riscv_interrupt_attribute) << 2;
+    return;
+  }
+
+  if (getFunctionOrMethodNumParams(D) != 0) {
+    S.Diag(D->getLocation(), diag::warn_riscv_interrupt_attribute) << 0;
+    return;
+  }
+
+  if (!getFunctionOrMethodResultType(D)->isVoidType()) {
+    S.Diag(D->getLocation(), diag::warn_riscv_interrupt_attribute) << 1;
+    return;
+  }
+
+  RISCVInterruptAttr::InterruptType Kind;
+  if (!RISCVInterruptAttr::ConvertStrToInterruptType(Str, Kind)) {
+    S.Diag(AL.getLoc(), diag::warn_attribute_type_not_supported)
+      << AL.getName() << Str << ArgLoc;
+    return;
+  }
+
+  D->addAttr(::new (S.Context) RISCVInterruptAttr(
+    AL.getLoc(), S.Context, Kind, AL.getAttributeSpellingListIndex()));
+}
+
 static void handleInterruptAttr(Sema &S, Decl *D, const AttributeList &AL) {
   // Dispatch the interrupt attribute based on the current target.
   switch (S.Context.getTargetInfo().getTriple().getArch()) {
   case llvm::Triple::msp430:
     handleMSP430InterruptAttr(S, D, AL);
     break;
   case llvm::Triple::mipsel:
   case llvm::Triple::mips:
     handleMipsInterruptAttr(S, D, AL);
     break;
   case llvm::Triple::x86:
   case llvm::Triple::x86_64:
     handleAnyX86InterruptAttr(S, D, AL);
     break;
   case llvm::Triple::avr:
     handleAVRInterruptAttr(S, D, AL);
     break;
+  case llvm::Triple::riscv32:
+  case llvm::Triple::riscv64:
+    handleRISCVInterruptAttr(S, D, AL);
+    break;
   default:
     handleARMInterruptAttr(S, D, AL);
     break;
   }
 }
 
 static void handleAMDGPUFlatWorkGroupSizeAttr(Sema &S, Decl *D,
                                               const AttributeList &AL) {

test/Sema/riscv-interrupt-attr.c

+// RUN: %clang_cc1 %s -triple riscv32-unknown-elf -verify -fsyntax-only
+// RUN: %clang_cc1 %s -triple riscv64-unknown-elf -verify -fsyntax-only
+
+struct a { int b; };
+
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to functions}}
+
+__attribute__((interrupt("USER"))) void foo1(void) {} // expected-warning {{'interrupt' attribute argument not supported: USER}}
+
+__attribute__((interrupt("user", 1))) void foo2(void) {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt)) int foo3(void) {return 0;} // expected-warning {{RISC-V 'interrupt' attribute only applies to functions that have a 'void' return type}}
+
+__attribute__((interrupt())) int foo4(); // expected-warning {{RISC-V 'interrupt' attribute only applies to functions that have a prototype}}
+
+__attribute__((interrupt())) int foo4() {} // expected-warning {{RISC-V 'interrupt' attribute only applies to functions that have a prototype}}
+
+__attribute__((interrupt())) void foo5(int a) {} // expected-warning {{RISC-V 'interrupt' attribute only applies to functions that have no parameters}}
+
+__attribute__((interrupt("user"), interrupt("supervisor"))) void foo6(void) {} // expected-warning {{repeated RISC-V 'interrupt' attribute}} \
+  // expected-note {{repeated RISC-V 'interrupt' attribute is here}}
+
+__attribute__((interrupt, interrupt)) void foo7(void) {} // expected-warning {{repeated RISC-V 'interrupt' attribute}} \
+                                                     // expected-note {{repeated RISC-V 'interrupt' attribute is here}}
+
+__attribute__((interrupt(""))) void foo8(void) {} // expected-warning {{'interrupt' attribute argument not supported}}
+
+__attribute__((interrupt("user"))) void foo9(void);
+__attribute__((interrupt("supervisor"))) void foo9(void);
+__attribute__((interrupt("machine"))) void foo9(void);
+
+__attribute__((interrupt("user"))) void foo10(void) {}
+__attribute__((interrupt("supervisor"))) void foo11(void) {}
+__attribute__((interrupt("machine"))) void foo12(void) {}
+__attribute__((interrupt())) void foo13(void) {}
+__attribute__((interrupt)) void foo14(void) {}
+

test/Sema/riscv-interrupt-attr.cpp

+// RUN: %clang_cc1 %s -triple riscv32-unknown-elf -verify -fsyntax-only
+// RUN: %clang_cc1 %s -triple riscv64-unknown-elf -verify -fsyntax-only
+
+[[gnu::interrupt]] [[gnu::interrupt]] void foo1() {} // expected-warning {{repeated RISC-V 'interrupt' attribute}} \
+                                                     // expected-note {{repeated RISC-V 'interrupt' attribute is here}}
+[[gnu::interrupt]] void foo2() {}
+