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

[Simon Dardis via cfe-commits]

sdardis updated this revision to Diff 41283.
sdardis added a comment.

Nit addressed.

Daniel or Aaron, can one of you commit on my behalf? Thanks.

Aaron, thanks for the review.


http://reviews.llvm.org/D10802

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/mips-interrupt-attr.c
  test/Sema/mips-interrupt-attr.c

Index: test/Sema/mips-interrupt-attr.c
===
--- /dev/null
+++ test/Sema/mips-interrupt-attr.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 %s -triple mips-img-elf -verify -fsyntax-only
+struct a { int b; };
+
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to functions and methods}}
+
+__attribute__((interrupt("EIC"))) void foo1() {} // expected-warning {{'interrupt' attribute argument not supported: 'EIC'}}
+
+__attribute__((interrupt("eic", 1))) void foo2() {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt("eic"))) void foo3() {}
+__attribute__((interrupt("vector=sw0"))) void foo4() {}
+__attribute__((interrupt("vector=hw0"))) void foo5() {}
+__attribute__((interrupt("vector=hw1"))) void foo6() {}
+__attribute__((interrupt("vector=hw2"))) void foo7() {}
+__attribute__((interrupt("vector=hw3"))) void foo8() {}
+__attribute__((interrupt("vector=hw4"))) void foo9() {}
+__attribute__((interrupt("vector=hw5"))) void fooa() {}
+__attribute__((interrupt(""))) void food() {}
+
+__attribute__((interrupt)) int foob() {return 0;} // expected-warning {{MIPS 'interrupt' attribute only applies to functions that have a 'void' return type}}
+__attribute__((interrupt())) void fooc(int a) {} // expected-warning {{MIPS 'interrupt' attribute only applies to functions that have no parameters}}
+__attribute__((interrupt,mips16)) void fooe() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+__attribute__((mips16,interrupt)) void foof() {} // expected-error {{'mips16' and 'interrupt' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+__attribute__((interrupt)) __attribute__ ((mips16)) void foo10() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+__attribute__((mips16)) __attribute ((interrupt)) void foo11() {} // expected-error {{'mips16' and 'interrupt' attributes are not compatible}} \
+  // expected-note {{conflicting attribute is here}}
Index: test/CodeGen/mips-interrupt-attr.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr.c
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (void)
+{
+  // CHECK: define void @isr_sw0() [[SW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=sw1")))
+isr_sw1 (void)
+{
+  // CHECK: define void @isr_sw1() [[SW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw0")))
+isr_hw0 (void)
+{
+  // CHECK: define void @isr_hw0() [[HW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw1")))
+isr_hw1 (void)
+{
+  // CHECK: define void @isr_hw1() [[HW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw2")))
+isr_hw2 (void)
+{
+  // CHECK: define void @isr_hw2() [[HW2:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw3")))
+isr_hw3 (void)
+{
+  // CHECK: define void @isr_hw3() [[HW3:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw4")))
+isr_hw4 (void)
+{
+  // CHECK: define void @isr_hw4() [[HW4:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw5")))
+isr_hw5 (void)
+{
+  // CHECK: define void @isr_hw5() [[HW5:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt))
+isr_eic (void)
+{
+  // CHECK: define void @isr_eic() [[EIC:#[0-9]+]]
+}
+// CHECK: attributes [[SW0]] = { {{.*}} "interrupt"="sw0" {{.*}} }
+// CHECK: attributes [[SW1]] = { {{.*}} "interrupt"="sw1" {{.*}} }
+// CHECK: attributes [[HW0]] = { {{.*}} "interrupt"="hw0" {{.*}} }
+// CHECK: attributes [[HW1]] = { {{.*}} "interrupt"="hw1" {{.*}} }
+// CHECK: attributes [[HW2]] = { {{.*}} "interrupt"="hw2" {{.*}} }
+// CHECK: attributes [[HW3]] = { {{.*}} "interrupt"="hw3" {{.*}} }
+// CHECK: attributes [[HW4]] = { {{.*}} "interrupt"="hw4" {{.*}} }
+// CHECK: attributes [[HW5]] = { {{.*}} "interrupt"="hw5" {{.*}} }
+// CHECK: attributes [[EIC]] = { {{.*}} "interrupt"="eic" {{.*}} }
Index: lib/Sema/SemaDeclAttr.cpp

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

[Aaron Ballman via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, once one minor nit is fixed. Thank you for working on this!



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:257
@@ +256,3 @@
+   "MIPS 'interrupt' attribute only applies to functions that have "
+   "%select{no parameters|a \'void\' return type}0">,
+   InGroup;

Spurious \' around 'void'


http://reviews.llvm.org/D10802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Simon Dardis via cfe-commits]

sdardis updated this revision to Diff 41041.
sdardis added a comment.

Updated text of return type/parameters warning to Aaron's suggestion. 
Dropped '\' escape characters from warning/error text. 
Removed mips16/nomips16 check.
Tweak of comment in handleMipsInterruptAttr.


http://reviews.llvm.org/D10802

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/mips-interrupt-attr.c
  test/Sema/mips-interrupt-attr.c

Index: test/Sema/mips-interrupt-attr.c
===
--- /dev/null
+++ test/Sema/mips-interrupt-attr.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 %s -triple mips-img-elf -verify -fsyntax-only
+struct a { int b; };
+
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to functions and methods}}
+
+__attribute__((interrupt("EIC"))) void foo1() {} // expected-warning {{'interrupt' attribute argument not supported: 'EIC'}}
+
+__attribute__((interrupt("eic", 1))) void foo2() {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt("eic"))) void foo3() {}
+__attribute__((interrupt("vector=sw0"))) void foo4() {}
+__attribute__((interrupt("vector=hw0"))) void foo5() {}
+__attribute__((interrupt("vector=hw1"))) void foo6() {}
+__attribute__((interrupt("vector=hw2"))) void foo7() {}
+__attribute__((interrupt("vector=hw3"))) void foo8() {}
+__attribute__((interrupt("vector=hw4"))) void foo9() {}
+__attribute__((interrupt("vector=hw5"))) void fooa() {}
+__attribute__((interrupt(""))) void food() {}
+
+__attribute__((interrupt)) int foob() {return 0;} // expected-warning {{MIPS 'interrupt' attribute only applies to functions that have a 'void' return type}}
+__attribute__((interrupt())) void fooc(int a) {} // expected-warning {{MIPS 'interrupt' attribute only applies to functions that have no parameters}}
+__attribute__((interrupt,mips16)) void fooe() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+__attribute__((mips16,interrupt)) void foof() {} // expected-error {{'mips16' and 'interrupt' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+__attribute__((interrupt)) __attribute__ ((mips16)) void foo10() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+__attribute__((mips16)) __attribute ((interrupt)) void foo11() {} // expected-error {{'mips16' and 'interrupt' attributes are not compatible}} \
+  // expected-note {{conflicting attribute is here}}
Index: test/CodeGen/mips-interrupt-attr.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr.c
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (void)
+{
+  // CHECK: define void @isr_sw0() [[SW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=sw1")))
+isr_sw1 (void)
+{
+  // CHECK: define void @isr_sw1() [[SW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw0")))
+isr_hw0 (void)
+{
+  // CHECK: define void @isr_hw0() [[HW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw1")))
+isr_hw1 (void)
+{
+  // CHECK: define void @isr_hw1() [[HW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw2")))
+isr_hw2 (void)
+{
+  // CHECK: define void @isr_hw2() [[HW2:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw3")))
+isr_hw3 (void)
+{
+  // CHECK: define void @isr_hw3() [[HW3:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw4")))
+isr_hw4 (void)
+{
+  // CHECK: define void @isr_hw4() [[HW4:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw5")))
+isr_hw5 (void)
+{
+  // CHECK: define void @isr_hw5() [[HW5:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt))
+isr_eic (void)
+{
+  // CHECK: define void @isr_eic() [[EIC:#[0-9]+]]
+}
+// CHECK: attributes [[SW0]] = { {{.*}} "interrupt"="sw0" {{.*}} }
+// CHECK: attributes [[SW1]] = { {{.*}} "interrupt"="sw1" {{.*}} }
+// CHECK: attributes [[HW0]] = { {{.*}} "interrupt"="hw0" {{.*}} }
+// CHECK: attributes [[HW1]] = { {{.*}} "interrupt"="hw1" {{.*}} }
+// CHECK: attributes [[HW2]] = { {{.*}} "interrupt"="hw2" {{.*}} }
+// CHECK: attributes [[HW3]] = { {{.*}} "interrupt"="hw3" {{.*}} }
+// CHECK: attributes [[HW4]] = { {{.*}} "interrupt"="hw4" {{.*}} }
+// CHECK: attributes [[HW5]] = { {{.*}} "interrupt"="hw5" {{.*}} }
+// CHECK: attributes [[EIC]] = { {{.*}} "interrupt"="eic" {{.*}} 

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

[Simon Dardis via cfe-commits]

sdardis updated this revision to Diff 40782.
sdardis marked 4 inline comments as done.
sdardis added a comment.

Updated comments, used cast as suggested, added mutual exclusion check for 
mips16+interrupt combination.

Extended mips16/nomips16 attribute handlers for mutual exclusion, add to 
pre-existing test.

Thanks.


http://reviews.llvm.org/D10802

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/mips-interrupt-attr.c
  test/Sema/mips-interrupt-attr.c
  test/Sema/mips16_attr_allowed.c

Index: test/Sema/mips16_attr_allowed.c
===
--- test/Sema/mips16_attr_allowed.c
+++ test/Sema/mips16_attr_allowed.c
@@ -19,6 +19,9 @@
 void __attribute__((nomips16(1, 2))) foo32b(); // expected-error {{'nomips16' attribute takes no arguments}}
 void __attribute__((mips16(1, 2))) foo16b(); // expected-error {{'mips16' attribute takes no arguments}}
 
+void __attribute__((mips16, nomips16)) foo16c(); // expected-error {{'mips16' and 'nomips16' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+
 
 __attribute((nomips16)) int a; // expected-error {{attribute only applies to functions}}
 
Index: test/Sema/mips-interrupt-attr.c
===
--- /dev/null
+++ test/Sema/mips-interrupt-attr.c
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 %s -triple mips-img-elf -verify -fsyntax-only
+struct a { int b; };
+
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to functions and methods}}
+
+__attribute__((interrupt("EIC"))) void foo1() {} // expected-warning {{'interrupt' attribute argument not supported: 'EIC'}}
+
+__attribute__((interrupt("eic", 1))) void foo2() {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt("eic"))) void foo3() {}
+__attribute__((interrupt("vector=sw0"))) void foo4() {}
+__attribute__((interrupt("vector=hw0"))) void foo5() {}
+__attribute__((interrupt("vector=hw1"))) void foo6() {}
+__attribute__((interrupt("vector=hw2"))) void foo7() {}
+__attribute__((interrupt("vector=hw3"))) void foo8() {}
+__attribute__((interrupt("vector=hw4"))) void foo9() {}
+__attribute__((interrupt("vector=hw5"))) void fooa() {}
+__attribute__((interrupt(""))) void food() {}
+
+__attribute__((interrupt)) int foob() {return 0;} // expected-warning {{function 'foob' must have the 'void' return type for the 'interrupt' attribute for MIPS}}
+__attribute__((interrupt())) void fooc(int a) {} // expected-warning {{function 'fooc' must take no arguments for the 'interrupt' attribute for MIPS}}
+__attribute__((interrupt,mips16)) void fooe() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+__attribute__((mips16,interrupt)) void foof() {} // expected-error {{'mips16' and 'interrupt' attributes are not compatible}} \
+ // expected-note {{conflicting attribute is here}}
+__attribute__((interrupt)) __attribute__ ((mips16)) void foo10() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}} \
+// expected-note {{conflicting attribute is here}}
+__attribute__((mips16)) __attribute ((interrupt)) void foo11() {} // expected-error {{'mips16' and 'interrupt' attributes are not compatible}} \
+  // expected-note {{conflicting attribute is here}}
Index: test/CodeGen/mips-interrupt-attr.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr.c
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (void)
+{
+  // CHECK: define void @isr_sw0() [[SW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=sw1")))
+isr_sw1 (void)
+{
+  // CHECK: define void @isr_sw1() [[SW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw0")))
+isr_hw0 (void)
+{
+  // CHECK: define void @isr_hw0() [[HW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw1")))
+isr_hw1 (void)
+{
+  // CHECK: define void @isr_hw1() [[HW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw2")))
+isr_hw2 (void)
+{
+  // CHECK: define void @isr_hw2() [[HW2:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw3")))
+isr_hw3 (void)
+{
+  // CHECK: define void @isr_hw3() [[HW3:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw4")))
+isr_hw4 (void)
+{
+  // CHECK: define void @isr_hw4() [[HW4:#[0-9]+]]
+}
+
+void 

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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:256
@@ +255,3 @@
+def warn_mips_interrupt_attribute : Warning<
+   "function %0 must %select{take no arguments|have the \'void\' return type}1"
+   " for the 'interrupt' attribute for MIPS">,

No need for \'; you can use ' directly.

I think the wording is still a bit awkward. Function declarations generally 
don't "take arguments", but have parameters, and "the void return type" reads 
weird.  I think:

"MIPS 'interrupt' attribute only applies to functions with %select{no 
parameters|a 'void' return type}0"

would be an improvement.


Comment at: lib/Sema/SemaDeclAttr.cpp:4439
@@ +4438,3 @@
+S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+<< "\'interrupt\'" << ExpectedFunctionOrMethod;
+return;

No need for \', you can use ' directly.


Comment at: lib/Sema/SemaDeclAttr.cpp:4462
@@ +4461,3 @@
+S.Diag(Attr.getLoc(), diag::warn_attribute_type_not_supported)
+<< Attr.getName() << "\'" + std::string(Str) + "\'";
+return;

No need for \', can use ' directly.


Comment at: lib/Sema/SemaDeclAttr.cpp:4484
@@ +4483,3 @@
+static void handleMips16Attribute(Sema , Decl *D, const AttributeList ) 
{
+  if (checkAttrMutualExclusion(S, D, Attr.getRange(), 
Attr.getName()))
+return;

This is a good change, but should be as part of a separate patch since it has 
nothing to do with MIPS interrupt.


Comment at: lib/Sema/SemaDeclAttr.cpp:4495
@@ +4494,3 @@
+static void handleNoMips16Attribute(Sema , Decl *D, const AttributeList 
) {
+  if (checkAttrMutualExclusion(S, D, Attr.getRange(), 
Attr.getName()))
+return;

Same with this.


Comment at: test/Sema/mips16_attr_allowed.c:22
@@ -21,1 +21,3 @@
 
+void __attribute__((mips16, nomips16)) foo16c(); // expected-error {{'mips16' 
and 'nomips16' attributes are not compatible}} \
+ // expected-note 
{{conflicting attribute is here}}

This should be part of a separate patch.


http://reviews.llvm.org/D10802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Simon Dardis via cfe-commits]

sdardis updated this revision to Diff 40382.
sdardis marked 12 inline comments as done.
sdardis added a comment.

Updated documentation text.

Switched the various hard failures to semantic warnings/failures.

Strangely, the 'interrupt' attribute on a non-function defaults to a warning.

Thanks.


http://reviews.llvm.org/D10802

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/mips-interrupt-attr.c
  test/Sema/mips-interrupt-attr.c

Index: test/Sema/mips-interrupt-attr.c
===
--- /dev/null
+++ test/Sema/mips-interrupt-attr.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 %s -triple mips-img-elf -verify -fsyntax-only
+struct a { int b; };
+
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt' attribute only applies to functions and methods}}
+
+__attribute__((interrupt("EIC"))) void foo1() {} // expected-warning {{'interrupt' attribute argument not supported: 'EIC'}}
+
+__attribute__((interrupt("eic", 1))) void foo2() {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt("eic"))) void foo3() {}
+__attribute__((interrupt("vector=sw0"))) void foo4() {}
+__attribute__((interrupt("vector=hw0"))) void foo5() {}
+__attribute__((interrupt("vector=hw1"))) void foo6() {}
+__attribute__((interrupt("vector=hw2"))) void foo7() {}
+__attribute__((interrupt("vector=hw3"))) void foo8() {}
+__attribute__((interrupt("vector=hw4"))) void foo9() {}
+__attribute__((interrupt("vector=hw5"))) void fooa() {}
+
+__attribute__((interrupt)) int foob() {} // expected-error {{function 'foob' does not have the 'void' return type}}
+__attribute__((interrupt())) void fooc(int a) {} // expected-error {{function 'fooc' has too many arguments}}
+__attribute__((interrupt(""))) void food() {}
+__attribute__((interrupt,mips16)) void fooe() {} // expected-error {{'interrupt' and 'mips16' attributes are not compatible}}
Index: test/CodeGen/mips-interrupt-attr.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr.c
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (void)
+{
+  // CHECK: define void @isr_sw0() [[SW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=sw1")))
+isr_sw1 (void)
+{
+  // CHECK: define void @isr_sw1() [[SW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw0")))
+isr_hw0 (void)
+{
+  // CHECK: define void @isr_hw0() [[HW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw1")))
+isr_hw1 (void)
+{
+  // CHECK: define void @isr_hw1() [[HW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw2")))
+isr_hw2 (void)
+{
+  // CHECK: define void @isr_hw2() [[HW2:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw3")))
+isr_hw3 (void)
+{
+  // CHECK: define void @isr_hw3() [[HW3:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw4")))
+isr_hw4 (void)
+{
+  // CHECK: define void @isr_hw4() [[HW4:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw5")))
+isr_hw5 (void)
+{
+  // CHECK: define void @isr_hw5() [[HW5:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt))
+isr_eic (void)
+{
+  // CHECK: define void @isr_eic() [[EIC:#[0-9]+]]
+}
+// CHECK: attributes [[SW0]] = { {{.*}} "interrupt"="sw0" {{.*}} }
+// CHECK: attributes [[SW1]] = { {{.*}} "interrupt"="sw1" {{.*}} }
+// CHECK: attributes [[HW0]] = { {{.*}} "interrupt"="hw0" {{.*}} }
+// CHECK: attributes [[HW1]] = { {{.*}} "interrupt"="hw1" {{.*}} }
+// CHECK: attributes [[HW2]] = { {{.*}} "interrupt"="hw2" {{.*}} }
+// CHECK: attributes [[HW3]] = { {{.*}} "interrupt"="hw3" {{.*}} }
+// CHECK: attributes [[HW4]] = { {{.*}} "interrupt"="hw4" {{.*}} }
+// CHECK: attributes [[HW5]] = { {{.*}} "interrupt"="hw5" {{.*}} }
+// CHECK: attributes [[EIC]] = { {{.*}} "interrupt"="eic" {{.*}} }
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4408,10 +4408,74 @@
   D->addAttr(UsedAttr::CreateImplicit(S.Context));
 }
 
+static void handleMipsInterruptAttr(Sema , Decl *D,
+const AttributeList ) {
+  // Only one optional argument permitted.
+  if (Attr.getNumArgs() > 1) {
+S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
+<< Attr.getName() << 1;
+return;
+  }
+
+  StringRef Str;
+  SourceLocation ArgLoc;
+
+  if (Attr.getNumArgs() == 0)
+Str = "";
+  else if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, ))
+return;
+
+
+  // Semantic checks for a function with the 'interrupt' attribute.
+  // a) Must be a function.
+  // b) Must take no arguments.
+  // c) Must have the 'void' return type.
+  // d) Cannot also 

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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: include/clang/Basic/DiagnosticSemaKinds.td:255
@@ -254,1 +254,3 @@
 def err_parameter_name_omitted : Error<"parameter name omitted">;
+def err_excess_arguments : Error<"function %0 has too many arguments">;
+def err_wrong_return_type : Error<"function %0 does not have the \'void\' 
return type">;

I would combine these, and name the result so that it is obvious it relates to 
the MIPS interrupt attribute. This can then become: "MIPS 'interrupt' attribute 
only applies to functions %select{with no parameters|a 'void' return type}0" 
Generally speaking, we make these warnings instead of errors (though the choice 
is obviously yours), and add them to the IgnoredAttributes group (so you warn, 
and then never attach the attribute to the declaration).


Comment at: lib/Sema/SemaDeclAttr.cpp:4441
@@ +4440,3 @@
+
+  DeclarationName Name = D->getAsFunction()->getNameInfo().getName();
+

No need for this; you can pass any NamedDecl * into Diag() and it will get 
quoted and displayed properly. Since you've already checked that it's a 
function or method, you can simply use cast(D) when needed.


Comment at: lib/Sema/SemaDeclAttr.cpp:4448
@@ +4447,3 @@
+
+  if (!(getFunctionOrMethodResultType(D)->isVoidType())) {
+S.Diag(D->getLocation(), diag::err_wrong_return_type) << Name;

No need for the extra set of parens.


Comment at: lib/Sema/SemaDeclAttr.cpp:4453
@@ +4452,3 @@
+
+  if (D->hasAttr()) {
+S.Diag(Attr.getLoc(), diag::err_attributes_are_not_compatible)

Please use checkAttrMutualExclusion() instead. Also, you need to add a similar 
check to the Mips16 attribute handler to ensure the mutual exclusion is 
properly checked. Should have semantic test cases for both orderings:
```
__attribute__((mips16)) __attribute__((interrupt)) void f();
__attribute__((interrupt)) __attribute__((mips16)) void g();
```
What about the nomips16 attribute? Should this be mutually exclusive as well?


http://reviews.llvm.org/D10802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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();
+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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Simon Dardis via cfe-commits]

sdardis updated this revision to Diff 38533.
sdardis added a comment.

Updated tests along the lines of what vkalintiris did for llvm.


http://reviews.llvm.org/D10802

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/mips-interrupt-attr-arg.c
  test/CodeGen/mips-interrupt-attr.c
  test/Sema/mips-interrupt-attr.c

Index: test/Sema/mips-interrupt-attr.c
===
--- /dev/null
+++ test/Sema/mips-interrupt-attr.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -triple mips-img-elf -verify -fsyntax-only
+
+__attribute__((interrupt("EIC"))) void foo1() {} // expected-warning {{'interrupt' attribute argument not supported: EIC}}
+
+__attribute__((interrupt("eic", 1))) void foo2() {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt("eic"))) void foo3() {}
+__attribute__((interrupt("vector=sw0"))) void foo4() {}
+__attribute__((interrupt("vector=hw0"))) void foo5() {}
+__attribute__((interrupt("vector=hw1"))) void foo6() {}
+__attribute__((interrupt("vector=hw2"))) void foo7() {}
+__attribute__((interrupt("vector=hw3"))) void foo8() {}
+__attribute__((interrupt("vector=hw4"))) void foo9() {}
+__attribute__((interrupt("vector=hw5"))) void fooa() {}
+
+__attribute__((interrupt)) void foob() {}
+__attribute__((interrupt())) void fooc() {}
+__attribute__((interrupt(""))) void food() {}
Index: test/CodeGen/mips-interrupt-attr.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr.c
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (void)
+{
+  // CHECK: define void @isr_sw0() [[SW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=sw1")))
+isr_sw1 (void)
+{
+  // CHECK: define void @isr_sw1() [[SW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw0")))
+isr_hw0 (void)
+{
+  // CHECK: define void @isr_hw0() [[HW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw1")))
+isr_hw1 (void)
+{
+  // CHECK: define void @isr_hw1() [[HW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw2")))
+isr_hw2 (void)
+{
+  // CHECK: define void @isr_hw2() [[HW2:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw3")))
+isr_hw3 (void)
+{
+  // CHECK: define void @isr_hw3() [[HW3:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw4")))
+isr_hw4 (void)
+{
+  // CHECK: define void @isr_hw4() [[HW4:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw5")))
+isr_hw5 (void)
+{
+  // CHECK: define void @isr_hw5() [[HW5:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt))
+isr_eic (void)
+{
+  // CHECK: define void @isr_eic() [[EIC:#[0-9]+]]
+}
+// CHECK: attributes [[SW0]] = { {{.*}} "interrupt"="sw0" {{.*}} }
+// CHECK: attributes [[SW1]] = { {{.*}} "interrupt"="sw1" {{.*}} }
+// CHECK: attributes [[HW0]] = { {{.*}} "interrupt"="hw0" {{.*}} }
+// CHECK: attributes [[HW1]] = { {{.*}} "interrupt"="hw1" {{.*}} }
+// CHECK: attributes [[HW2]] = { {{.*}} "interrupt"="hw2" {{.*}} }
+// CHECK: attributes [[HW3]] = { {{.*}} "interrupt"="hw3" {{.*}} }
+// CHECK: attributes [[HW4]] = { {{.*}} "interrupt"="hw4" {{.*}} }
+// CHECK: attributes [[HW5]] = { {{.*}} "interrupt"="hw5" {{.*}} }
+// CHECK: attributes [[EIC]] = { {{.*}} "interrupt"="eic" {{.*}} }
Index: test/CodeGen/mips-interrupt-attr-arg.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr-arg.c
@@ -0,0 +1,8 @@
+// RUN: not %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s >2 %t
+// FileCheck %s < %t
+
+; CHECK: LLVM ERROR: Functions with the interrupt attribute cannot have arguments!
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (char * a)
+{
+}
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4251,10 +4251,44 @@
   D->addAttr(UsedAttr::CreateImplicit(S.Context));
 }
 
+static void handleMipsInterruptAttr(Sema , Decl *D,
+const AttributeList ) {
+  // Only one optional argument permitted.
+  if (Attr.getNumArgs() > 1) {
+S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
+<< Attr.getName() << 1;
+return;
+  }
+
+  StringRef Str;
+  SourceLocation ArgLoc;
+
+  if (Attr.getNumArgs() == 0)
+Str = "";
+  else if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, ))
+return;
+
+  MipsInterruptAttr::InterruptType Kind;
+  if (!MipsInterruptAttr::ConvertStrToInterruptType(Str, Kind)) {
+S.Diag(Attr.getLoc(), diag::warn_attribute_type_not_supported)
+<< Attr.getName() << Str << ArgLoc;
+return;
+  }
+
+  unsigned Index = Attr.getAttributeSpellingListIndex();
+  D->addAttr(::new (S.Context)
+  

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

[Simon Dardis via cfe-commits]

sdardis updated the summary for this revision.
sdardis updated this revision to Diff 38102.
sdardis marked 3 inline comments as done.
sdardis added a comment.

Nits addressed and XFAIL tests added for functions having arguments and the 
interrupt attribute.

Test added to cover semantic warnings.

Rejects functions with mips16 & interrupt attributes.

Text in the bullet points of MipsInterruptDocs section expanded.


http://reviews.llvm.org/D10802

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/TargetInfo.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/CodeGen/mips-interrupt-attr-arg.c
  test/CodeGen/mips-interrupt-attr.c
  test/Sema/mips-interrupt-attr.c

Index: test/Sema/mips-interrupt-attr.c
===
--- /dev/null
+++ test/Sema/mips-interrupt-attr.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -triple mips-img-elf -verify -fsyntax-only
+
+__attribute__((interrupt("EIC"))) void foo1() {} // expected-warning {{'interrupt' attribute argument not supported: EIC}}
+
+__attribute__((interrupt("eic", 1))) void foo2() {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+
+__attribute__((interrupt("eic"))) void foo3(int a) {}
+__attribute__((interrupt("vector=sw0"))) void foo4() {}
+__attribute__((interrupt("vector=hw0"))) void foo5() {}
+__attribute__((interrupt("vector=hw1"))) void foo6() {}
+__attribute__((interrupt("vector=hw2"))) void foo7() {}
+__attribute__((interrupt("vector=hw3"))) void foo8() {}
+__attribute__((interrupt("vector=hw4"))) void foo9() {}
+__attribute__((interrupt("vector=hw5"))) void fooa() {}
+
+__attribute__((interrupt)) void foob() {}
+__attribute__((interrupt())) void fooc() {}
+__attribute__((interrupt(""))) void food() {}
Index: test/CodeGen/mips-interrupt-attr.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr.c
@@ -0,0 +1,64 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (void)
+{
+  // CHECK: define void @isr_sw0() [[SW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=sw1")))
+isr_sw1 (void)
+{
+  // CHECK: define void @isr_sw1() [[SW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw0")))
+isr_hw0 (void)
+{
+  // CHECK: define void @isr_hw0() [[HW0:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw1")))
+isr_hw1 (void)
+{
+  // CHECK: define void @isr_hw1() [[HW1:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw2")))
+isr_hw2 (void)
+{
+  // CHECK: define void @isr_hw2() [[HW2:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw3")))
+isr_hw3 (void)
+{
+  // CHECK: define void @isr_hw3() [[HW3:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw4")))
+isr_hw4 (void)
+{
+  // CHECK: define void @isr_hw4() [[HW4:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt("vector=hw5")))
+isr_hw5 (void)
+{
+  // CHECK: define void @isr_hw5() [[HW5:#[0-9]+]]
+}
+
+void __attribute__ ((interrupt))
+isr_eic (void)
+{
+  // CHECK: define void @isr_eic() [[EIC:#[0-9]+]]
+}
+// CHECK: attributes [[SW0]] = { {{.*}} "interrupt"="sw0" {{.*}} }
+// CHECK: attributes [[SW1]] = { {{.*}} "interrupt"="sw1" {{.*}} }
+// CHECK: attributes [[HW0]] = { {{.*}} "interrupt"="hw0" {{.*}} }
+// CHECK: attributes [[HW1]] = { {{.*}} "interrupt"="hw1" {{.*}} }
+// CHECK: attributes [[HW2]] = { {{.*}} "interrupt"="hw2" {{.*}} }
+// CHECK: attributes [[HW3]] = { {{.*}} "interrupt"="hw3" {{.*}} }
+// CHECK: attributes [[HW4]] = { {{.*}} "interrupt"="hw4" {{.*}} }
+// CHECK: attributes [[HW5]] = { {{.*}} "interrupt"="hw5" {{.*}} }
+// CHECK: attributes [[EIC]] = { {{.*}} "interrupt"="eic" {{.*}} }
Index: test/CodeGen/mips-interrupt-attr-arg.c
===
--- /dev/null
+++ test/CodeGen/mips-interrupt-attr-arg.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple mipsel-unknown-linux -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK
+// XFAIL: *
+
+void __attribute__ ((interrupt("vector=sw0")))
+isr_sw0 (char * a)
+{
+  // CHECK: define void @isr_sw0(i8* %a) [[SW0:#[0-9]+]]
+}
+
+
+// CHECK: attributes [[SW0]] = { {{.*}} "interrupt"="sw0" {{.*}} }
+
Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -4251,10 +4251,44 @@
   D->addAttr(UsedAttr::CreateImplicit(S.Context));
 }
 
+static void handleMipsInterruptAttr(Sema , Decl *D,
+const AttributeList ) {
+  // Only one optional argument permitted.
+  if (Attr.getNumArgs() > 1) {
+S.Diag(Attr.getLoc(), diag::err_attribute_too_many_arguments)
+<< Attr.getName() << 1;
+return;
+  }
+
+  StringRef Str;
+  SourceLocation ArgLoc;
+
+  if (Attr.getNumArgs() == 0)
+Str = "";
+  else if (!S.checkStringLiteralArgumentAttr(Attr, 0, Str, 

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

[Daniel Sanders via cfe-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM with a few nits



Comment at: include/clang/Basic/Attr.td:841-842
@@ +840,4 @@
+def MipsInterrupt : InheritableAttr, TargetSpecificAttr {
+  // NOTE: if you add any additional spellings, ARMInterrupts's &
+  // MSP430Interrupt must match.
+  let Spellings = [GNU<"interrupt">];

if -> If
ARMInterrupts's -> ARMInterrupt's

Also, you should update the comments for ARMInterrupt and MSP430Interrupt to 
mention MipsInterrupt.


Comment at: include/clang/Basic/AttrDocs.td:690-691
@@ +689,4 @@
+The parameter passed to the interrupt attribute is optional, but if
+provided it must be a string literal with one of the following values: "sw0",
+"sw1", "hw0", "hw1", "hw2", "hw3", "hw4", "hw5" or "eic".
+

Aside from "eic" shouldn't these be "vector=..."?


Comment at: lib/Sema/SemaDeclAttr.cpp:4222
@@ -4192,3 +4221,3 @@
 handleMSP430InterruptAttr(S, D, Attr);
-  else
+  else if (S.Context.getTargetInfo().getTriple().getArch() == 
llvm::Triple::arm)
 handleARMInterruptAttr(S, D, Attr);

I think you also need to cover aarch64, thumb, etc. It's probably best to leave 
it as an else clause and add mips above it.


http://reviews.llvm.org/D10802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

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

Missing tests for the semantic warnings. For instance, I don't see any tests 
exercising err_attribute_too_many_arguments or 
warn_attribute_type_not_supported.

~Aaron


http://reviews.llvm.org/D10802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits