[PATCH] D48412: [RISCV] Add support for interrupt attribute

[Ana Pazos via Phabricator via cfe-commits]

apazos added inline comments.


================
Comment at: lib/CodeGen/TargetInfo.cpp:8966
+
+    const RISCVInterruptAttr *Attr = FD->getAttr<RISCVInterruptAttr>();
+    if (!Attr)
----------------
aaron.ballman wrote:
> You can use `const auto *` here instead of repeating the type.
Thanks Aaron, will do the cleanup.


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5280
+  // Check the attribute arguments.
+  if (AL.getNumArgs() > 1) {
+    S.Diag(AL.getLoc(), diag::err_attribute_too_many_arguments)
----------------
aaron.ballman wrote:
> 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).
The argument is optional and at most one argument , I will use checkAttributeAtMostNumArgs instead


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5301
+
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
----------------
aaron.ballman wrote:
> I don't think you need to perform this check -- I believe it's handled automatically (because you don't have custom parsing enabled).
I think need it. Will double check in the test.


================
Comment at: test/Sema/riscv-interrupt-attr.c:18
+
+__attribute__((interrupt("user"), interrupt("supervisor"))) void foo6() { } // expected-warning {{repeated RISC-V 'interrupt' attribute}} \
+                                                                            // expected-note {{repeated RISC-V 'interrupt' attribute is here}}
----------------
aaron.ballman wrote:
> 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() {}
> ```
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.


================
Comment at: test/Sema/riscv-interrupt-attr.c:26
+__attribute__((interrupt("machine"))) void foo10() {}
+__attribute__((interrupt(""))) void foo11() {}
+__attribute__((interrupt())) void foo12() {}
----------------
aaron.ballman wrote:
> 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.
Good catch, In include/clang/Basic/Attr.td I mapped "" to the default machine mode.
I will change it to align with GCC behavior.


https://reviews.llvm.org/D48412