[RISCV] Remove support for attribute interrupt("user").

[craig.topper]

Summary

This was part of the N extension which didn't make it version
1.12 of the privilege specification.

The uret instruction was removed in D149308.

[craig.topper]

Add another test case

[kito-cheng]

LGTM, let it go~~~~

[aaron.ballman]

Is this a potentially breaking change that we need to call out for users to be aware of?

[asb]

In D149314#4302203, @aaron.ballman wrote:

Is this a potentially breaking change that we need to call out for users to be aware of?

We should mention this in the Clang release notes I think.

[aaron.ballman]

In D149314#4302266, @asb wrote:

In D149314#4302203, @aaron.ballman wrote:

Is this a potentially breaking change that we need to call out for users to be aware of?

We should mention this in the Clang release notes I think.

Beyond that, we've got a process for what to do when considering potentially breaking changes, we should be following that: https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes Also, if we're all agreed this is potentially breaking, it really should have more review time and buy-in from the code owner.

[reames]

In D149314#4302300, @aaron.ballman wrote:

In D149314#4302266, @asb wrote:

In D149314#4302203, @aaron.ballman wrote:

Is this a potentially breaking change that we need to call out for users to be aware of?

We should mention this in the Clang release notes I think.

Beyond that, we've got a process for what to do when considering potentially breaking changes, we should be following that: https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes Also, if we're all agreed this is potentially breaking, it really should have more review time and buy-in from the code owner.

I don't think this change should count as "potentially breaking" in the sense of that document. We're talking about an experimental feature for an experimental ISA extension which never got to ratification. There's no hardware in the wild which implements this (to my knowledge). Given the churn on the RISCV extension side, we've adopted a policy for experimental extensions (https://llvm.org/docs/RISCVUsage.html#experimental-extensions) which offers much less in the way of support. I think we should release note it just to be friendly, but the process described in your link is significant overkill.

[aaron.ballman]

In D149314#4302312, @reames wrote:

In D149314#4302300, @aaron.ballman wrote:

In D149314#4302266, @asb wrote:

In D149314#4302203, @aaron.ballman wrote:

Is this a potentially breaking change that we need to call out for users to be aware of?

We should mention this in the Clang release notes I think.

Beyond that, we've got a process for what to do when considering potentially breaking changes, we should be following that: https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes Also, if we're all agreed this is potentially breaking, it really should have more review time and buy-in from the code owner.

I don't think this change should count as "potentially breaking" in the sense of that document. We're talking about an experimental feature for an experimental ISA extension which never got to ratification. There's no hardware in the wild which implements this (to my knowledge). Given the churn on the RISCV extension side, we've adopted a policy for experimental extensions (https://llvm.org/docs/RISCVUsage.html#experimental-extensions) which offers much less in the way of support. I think we should release note it just to be friendly, but the process described in your link is significant overkill.

If we don't need to use that process, it's totally reasonable to skip it. It wasn't clear just how disruptive this change is from the review summary and there was not really any chance for discussion before this landed. Release noting it would be fine by me if the folks with more knowledge in this area think that's sufficient.

[asb]

In D149314#4302312, @reames wrote:

In D149314#4302300, @aaron.ballman wrote:

In D149314#4302266, @asb wrote:

In D149314#4302203, @aaron.ballman wrote:

Is this a potentially breaking change that we need to call out for users to be aware of?

We should mention this in the Clang release notes I think.

Beyond that, we've got a process for what to do when considering potentially breaking changes, we should be following that: https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes Also, if we're all agreed this is potentially breaking, it really should have more review time and buy-in from the code owner.

I don't think this change should count as "potentially breaking" in the sense of that document. We're talking about an experimental feature for an experimental ISA extension which never got to ratification. There's no hardware in the wild which implements this (to my knowledge). Given the churn on the RISCV extension side, we've adopted a policy for experimental extensions (https://llvm.org/docs/RISCVUsage.html#experimental-extensions) which offers much less in the way of support. I think we should release note it just to be friendly, but the process described in your link is significant overkill.

+1 on this. The upstream (RISC-V side) process for the ISA extension lifecycle and their ratification is now properly established, and we gate not-yet-ratified things behind -menable-experimental-extensions going forward. This review perhaps could have been held open a bit longer to check there's no concerns, and thanks to Aaron for raising the question. But I think a release note only is the appropriate option here.

[aaron.ballman]

In D149314#4302342, @asb wrote:

In D149314#4302312, @reames wrote:

In D149314#4302300, @aaron.ballman wrote:

In D149314#4302266, @asb wrote:

In D149314#4302203, @aaron.ballman wrote:

Is this a potentially breaking change that we need to call out for users to be aware of?

We should mention this in the Clang release notes I think.

Beyond that, we've got a process for what to do when considering potentially breaking changes, we should be following that: https://llvm.org/docs/DeveloperPolicy.html#making-potentially-breaking-changes Also, if we're all agreed this is potentially breaking, it really should have more review time and buy-in from the code owner.

I don't think this change should count as "potentially breaking" in the sense of that document. We're talking about an experimental feature for an experimental ISA extension which never got to ratification. There's no hardware in the wild which implements this (to my knowledge). Given the churn on the RISCV extension side, we've adopted a policy for experimental extensions (https://llvm.org/docs/RISCVUsage.html#experimental-extensions) which offers much less in the way of support. I think we should release note it just to be friendly, but the process described in your link is significant overkill.

+1 on this. The upstream (RISC-V side) process for the ISA extension lifecycle and their ratification is now properly established, and we gate not-yet-ratified things behind -menable-experimental-extensions going forward. This review perhaps could have been held open a bit longer to check there's no concerns, and thanks to Aaron for raising the question. But I think a release note only is the appropriate option here.

SGTM, thank you for the discussion!

[craig.topper]

Release note added in 0e02e5decc732155d8dc4b63ecccbb1477603ecd

[aaron.ballman]

In D149314#4302892, @craig.topper wrote:

Release note added in 0e02e5decc732155d8dc4b63ecccbb1477603ecd

Thanks Craig, that looks great

clang/include/clang/Basic/Attr.td

def NoMicroMips : InheritableAttr, TargetSpecificAttr<TargetMips32> {
   let Documentation = [MicroMipsDocs];
   let SimpleHandler = 1;
 }
 
 def RISCVInterrupt : InheritableAttr, TargetSpecificAttr<TargetRISCV> {
   let Spellings = [GCC<"interrupt">];
   let Subjects = SubjectList<[Function]>;
   let Args = [EnumArgument<"Interrupt", "InterruptType",
-                           ["user", "supervisor", "machine"],
-                           ["user", "supervisor", "machine"],
+                           ["supervisor", "machine"],
+                           ["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.
 //

clang/lib/CodeGen/TargetInfo.cpp

void setTargetAttributes(const Decl *D, llvm::GlobalValue *GV,
     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);
   }

clang/test/Sema/riscv-interrupt-attr.c

 // RUN: %clang_cc1 -triple riscv32-unknown-elf -emit-llvm -DCHECK_IR < %s| FileCheck %s
 // RUN: %clang_cc1 -triple riscv64-unknown-elf -emit-llvm -DCHECK_IR < %s| FileCheck %s
 // RUN: %clang_cc1 %s -triple riscv32-unknown-elf -verify -fsyntax-only
 // RUN: %clang_cc1 %s -triple riscv64-unknown-elf -verify -fsyntax-only
 
 #if defined(CHECK_IR)
-// CHECK-LABEL:  @foo_user() #0
-// CHECK: ret void
-__attribute__((interrupt("user"))) void foo_user(void) {}
-// CHECK-LABEL:  @foo_supervisor() #1
+// CHECK-LABEL:  @foo_supervisor() #0
 // CHECK: ret void
 __attribute__((interrupt("supervisor"))) void foo_supervisor(void) {}
-// CHECK-LABEL:  @foo_machine() #2
+// CHECK-LABEL:  @foo_machine() #1
 // CHECK: ret void
 __attribute__((interrupt("machine"))) void foo_machine(void) {}
-// CHECK-LABEL:  @foo_default() #2
+// CHECK-LABEL:  @foo_default() #1
 // CHECK: ret void
 __attribute__((interrupt())) void foo_default(void) {}
-// CHECK-LABEL:  @foo_default2() #2
+// CHECK-LABEL:  @foo_default2() #1
 // CHECK: ret void
 __attribute__((interrupt())) void foo_default2(void) {}
 // CHECK: attributes #0
-// CHECK: "interrupt"="user"
-// CHECK: attributes #1
 // CHECK: "interrupt"="supervisor"
-// CHECK: attributes #2
+// CHECK: attributes #1
 // CHECK: "interrupt"="machine"
 #else
 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"))) void foo1b(void) {} // expected-warning {{'interrupt' attribute argument not supported: user}}
+__attribute__((interrupt("MACHINE"))) void foo1c(void) {} // expected-warning {{'interrupt' attribute argument not supported: MACHINE}}
 
-__attribute__((interrupt("user", 1))) void foo2(void) {} // expected-error {{'interrupt' attribute takes no more than 1 argument}}
+__attribute__((interrupt("machine", 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())) void foo4(void);
 __attribute__((interrupt())) void foo4(void) {}
 
 __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}} \
+__attribute__((interrupt("machine"), 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) {}
 #endif