[PATCH] D10802: [mips] Interrupt attribute support.

[Aaron Ballman via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

This is coming along nicely! There are some minor nits that should be trivial to fix, but the fatal errors need to become semantic errors, and some tests are missing.


================
Comment at: include/clang/Basic/AttrDocs.td:713
@@ +712,3 @@
+Clang supports the GNU style ``__attribute__((interrupt("ARGUMENT")))`` attribute on
+MIPS targets. This attribute may be attached ot a function definition and instructs
+the backend to generate appropriate function entry/exit code so that it can be used
----------------
s/ot/to

================
Comment at: include/clang/Basic/AttrDocs.td:717
@@ +716,3 @@
+
+By default the compiler will produce a function prologue and epilogue suitable for
+an interrupt service routine that handles an External Interrupt Controller (eic)
----------------
Comma after "By default"

================
Comment at: include/clang/Basic/AttrDocs.td:722
@@ +721,3 @@
+
+Otherwise, for use with vectored interrupt mode the argument passed should be
+of the form "vector=LEVEL" where LEVEL is one of the following values:
----------------
Comma after mode.

================
Comment at: include/clang/Basic/AttrDocs.td:738
@@ +737,3 @@
+
+- The FPU is disabled in the prologue as the floating pointer registers are not
+  spilled to the stack.
----------------
Comma after prologue.

================
Comment at: include/clang/Basic/AttrDocs.td:741
@@ +740,3 @@
+
+- Function return is changed to an exception return instruction.
+
----------------
The function return?

================
Comment at: lib/CodeGen/TargetInfo.cpp:5834
@@ +5833,3 @@
+
+    const MipsInterruptAttr * Attr = FD->getAttr<MipsInterruptAttr>();
+    if (!Attr)
----------------
No space between * and Attr. May want to run clang-format over the patch.

================
Comment at: lib/CodeGen/TargetInfo.cpp:5839
@@ +5838,3 @@
+    if (!Fn->arg_empty())
+      llvm::report_fatal_error(
+          "Functions with arguments and interrupt attribute not supported!");
----------------
This should be a semantic error for improved diagnostic behavior.

================
Comment at: lib/CodeGen/TargetInfo.cpp:5843
@@ +5842,3 @@
+    if (Fn->hasFnAttribute("mips16"))
+      llvm::report_fatal_error("Functions with the interrupt and mips16 "
+                               "attributes are not supported!");
----------------
Same here. Also, missing a semantic test for this.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4274
@@ +4273,3 @@
+    S.Diag(Attr.getLoc(), diag::warn_attribute_type_not_supported)
+        << Attr.getName() << Str << ArgLoc;
+    return;
----------------
Please have Str in quotes in the diagnostic (since it's something the user wrote).

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4278
@@ +4277,3 @@
+
+  unsigned Index = Attr.getAttributeSpellingListIndex();
+  D->addAttr(::new (S.Context)
----------------
Sink Index into the constructor arguments.

================
Comment at: test/CodeGen/mips-interrupt-attr-arg.c:5
@@ +4,3 @@
+; CHECK: LLVM ERROR: Functions with the interrupt attribute cannot have arguments!
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (char * a)
----------------
This should be folded into the other semantic test when this becomes a semantic error instead of a hard failure.

================
Comment at: test/Sema/mips-interrupt-attr.c:18
@@ +17,2 @@
+__attribute__((interrupt())) void fooc() {}
+__attribute__((interrupt(""))) void food() {}
----------------
Also missing semantic tests for placing the attribute on something other than a function (which I presume should be an error?).


http://reviews.llvm.org/D10802