[RISCV][VCIX] Add vcix_state to GNU inline assembly register set #106914

4vtomat · 2024-09-01T17:19:40Z

riscv-non-isa/riscv-toolchain-conventions#56
Resolved #106700.
This enables inline asm to have vcix_state to be a clobbered register
thus disable reordering between VCIX intrinsics and inline asm.

llvmbot · 2024-09-01T17:20:11Z

@llvm/pr-subscribers-clang

Author: Brandon Wu (4vtomat)

Changes

[RISCV][VCIX] Add vcix_state to GNU inline assembly register set

Resolved #106700.
This enables inline asm to have vcix_state to be a clobbered register
thus disable reordering between VCIX intrinsics and inline asm.

Full diff: https://github.com/llvm/llvm-project/pull/106914.diff

3 Files Affected:

(modified) clang/lib/Basic/Targets/RISCV.cpp (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVRegisterInfo.td (+5-1)
(added) llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll (+22)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index b89109e7725d44..da6ecfb4e4022b 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -44,7 +44,7 @@ ArrayRef<const char *> RISCVTargetInfo::getGCCRegNames() const {
       "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31",
 
       // CSRs
-      "fflags", "frm", "vtype", "vl", "vxsat", "vxrm"
+      "fflags", "frm", "vtype", "vl", "vxsat", "vxrm", "vcix_state"
     };
   // clang-format on
   return llvm::ArrayRef(GCCRegNames);
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
index 4d5c0a7bef9416..03f05c0baea3b0 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
@@ -664,5 +664,9 @@ def FRM    : RISCVReg<0, "frm">;
 // Shadow Stack register
 def SSP    : RISCVReg<0, "ssp">;
 
-// Dummy VCIX state register
+// Dummy VCIX state register and its register class
 def VCIX_STATE : RISCVReg<0, "vcix_state">;
+def : RISCVRegisterClass<[XLenVT], 32, (add VCIX_STATE)> {
+  let RegInfos = XLenRI;
+  let isAllocatable = 0;
+}
diff --git a/llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll b/llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll
new file mode 100644
index 00000000000000..0013461d873281
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll
@@ -0,0 +1,22 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: sed 's/iXLen/i32/g' %s | llc -mtriple=riscv32 -mattr=+v,+xsfvcp \
+; RUN:   -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK
+; RUN: sed 's/iXLen/i64/g' %s | llc -mtriple=riscv64 -mattr=+v,+xsfvcp \
+; RUN:   -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK
+
+; VCIX instructions can not reorder between each other.
+define void @test_reorder(<vscale x 1 x i64> %vreg) {
+; CHECK-LABEL: test_reorder:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    vsetivli zero, 0, e64, m1, ta, ma
+; CHECK-NEXT:    sf.vc.iv 0, 0, v8, 0
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    sf.vc.vv 3, 0, v8, v8
+; CHECK-EMPTY:
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    ret
+entry:
+  call void @llvm.riscv.sf.vc.iv.se.iXLen.nxv1i64.iXLen.iXLen(iXLen 0, iXLen 0, <vscale x 1 x i64> %vreg, iXLen 0, iXLen 0)
+  call iXLen asm sideeffect "sf.vc.vv 0x3, 0x0, $1, $1;", "=r,^vr,~{memory},~{vl},~{vcix_state}"(<vscale x 1 x i64> %vreg)
+  ret void
+}

llvmbot · 2024-09-01T17:20:11Z

@llvm/pr-subscribers-backend-risc-v

Author: Brandon Wu (4vtomat)

Changes

[RISCV][VCIX] Add vcix_state to GNU inline assembly register set

Resolved #106700.
This enables inline asm to have vcix_state to be a clobbered register
thus disable reordering between VCIX intrinsics and inline asm.

Full diff: https://github.com/llvm/llvm-project/pull/106914.diff

3 Files Affected:

(modified) clang/lib/Basic/Targets/RISCV.cpp (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVRegisterInfo.td (+5-1)
(added) llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll (+22)

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp
index b89109e7725d44..da6ecfb4e4022b 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -44,7 +44,7 @@ ArrayRef<const char *> RISCVTargetInfo::getGCCRegNames() const {
       "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31",
 
       // CSRs
-      "fflags", "frm", "vtype", "vl", "vxsat", "vxrm"
+      "fflags", "frm", "vtype", "vl", "vxsat", "vxrm", "vcix_state"
     };
   // clang-format on
   return llvm::ArrayRef(GCCRegNames);
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
index 4d5c0a7bef9416..03f05c0baea3b0 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
@@ -664,5 +664,9 @@ def FRM    : RISCVReg<0, "frm">;
 // Shadow Stack register
 def SSP    : RISCVReg<0, "ssp">;
 
-// Dummy VCIX state register
+// Dummy VCIX state register and its register class
 def VCIX_STATE : RISCVReg<0, "vcix_state">;
+def : RISCVRegisterClass<[XLenVT], 32, (add VCIX_STATE)> {
+  let RegInfos = XLenRI;
+  let isAllocatable = 0;
+}
diff --git a/llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll b/llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll
new file mode 100644
index 00000000000000..0013461d873281
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/inline-asm-xsfvcp.ll
@@ -0,0 +1,22 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: sed 's/iXLen/i32/g' %s | llc -mtriple=riscv32 -mattr=+v,+xsfvcp \
+; RUN:   -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK
+; RUN: sed 's/iXLen/i64/g' %s | llc -mtriple=riscv64 -mattr=+v,+xsfvcp \
+; RUN:   -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK
+
+; VCIX instructions can not reorder between each other.
+define void @test_reorder(<vscale x 1 x i64> %vreg) {
+; CHECK-LABEL: test_reorder:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    vsetivli zero, 0, e64, m1, ta, ma
+; CHECK-NEXT:    sf.vc.iv 0, 0, v8, 0
+; CHECK-NEXT:    #APP
+; CHECK-NEXT:    sf.vc.vv 3, 0, v8, v8
+; CHECK-EMPTY:
+; CHECK-NEXT:    #NO_APP
+; CHECK-NEXT:    ret
+entry:
+  call void @llvm.riscv.sf.vc.iv.se.iXLen.nxv1i64.iXLen.iXLen(iXLen 0, iXLen 0, <vscale x 1 x i64> %vreg, iXLen 0, iXLen 0)
+  call iXLen asm sideeffect "sf.vc.vv 0x3, 0x0, $1, $1;", "=r,^vr,~{memory},~{vl},~{vcix_state}"(<vscale x 1 x i64> %vreg)
+  ret void
+}

wangpc-pp · 2024-09-02T03:38:03Z

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

 def VCIX_STATE : RISCVReg<0, "vcix_state">;
+def : RISCVRegisterClass<[XLenVT], 32, (add VCIX_STATE)> {


Why do we need a RegisterClass for it?

In getRegForInlineAsmConstraint if there would be no value in AssignedRegs when calling getRegistersForValue for the vcix_state constraint, thus it would be deprecated when constructing selectionDAG.

kito-cheng · 2024-09-02T03:57:06Z

I would suggest it should prefix with a vendor prefix, either sf.vcix_state or sifive.vcix_state, also go riscv-c-api-doc or riscv-toolchain-conventions :)

4vtomat · 2024-09-02T05:54:09Z

I would suggest it should prefix with a vendor prefix, either sf.vcix_state or sifive.vcix_state, also go riscv-c-api-doc or riscv-toolchain-conventions :)

Do you mean change the current vcix_state register to sf.vcix_state?

kito-cheng · 2024-09-02T13:31:32Z

Do you mean change the current vcix_state register to sf.vcix_state?

Yes, because it's SiFive specific register, other vendor may add other status register like VCIX in future, so I would like to add prefix to make sure all further similar stuff will follow same rule if possible

4vtomat · 2024-09-02T13:47:14Z

Do you mean change the current vcix_state register to sf.vcix_state?

Yes, because it's SiFive specific register, other vendor may add other status register like VCIX in future, so I would like to add prefix to make sure all further similar stuff will follow same rule if possible

Sure! I will do it in another patch since it's separate thing than this patch.

kito-cheng · 2024-09-05T10:10:43Z

clang/lib/Basic/Targets/RISCV.cpp

@@ -44,7 +44,7 @@ ArrayRef<const char *> RISCVTargetInfo::getGCCRegNames() const {
      "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31",

      // CSRs
-      "fflags", "frm", "vtype", "vl", "vxsat", "vxrm"
+      "fflags", "frm", "vtype", "vl", "vxsat", "vxrm", "sf_vcix_state"


sf.vcix_state?

This is machine register name lol, if we use sf.vcix_state, it can't recognize.

Should we stay with the sf_ prefix then? Or is there some simple workaround?

What do you mean by "This is a machine instruction name [...] it can't recognize"? That's the name it's given in TableGen.

#include <riscv_vector.h> #include <sifive_vector.h> int foo(__rvv_uint64m1_t vreg) { auto vl = __riscv_vsetvl_e64m1(1); // This VCIX instruction gets scheduled after the asm block below. __riscv_sf_vc_iv_se_u64m1(0, 0, vreg, 0, vl); asm volatile( R"( vsetivli zero, 2, e64, m1, ta, ma sf.vc.vv 0x3, 0x0, %[vreg], %[vreg]; )" : "=r"(vl) : [vreg]"vr"(vreg) : "memory", "vl", "sf_vcix_state"); return 0; }

Use the code above as an example, it compiles to the llvm:

define dso_local noundef signext i32 @_Z3foou16__rvv_uint64m1_t(<vscale x 1 x i64> %vreg) local_unnamed_addr #0 { entry: %0 = tail call i64 @llvm.riscv.vsetvli.i64(i64 1, i64 3, i64 0) tail call void @llvm.riscv.sf.vc.iv.se.i64.nxv1i64.i64.i64(i64 0, i64 0, <vscale x 1 x i64> %vreg, i64 0, i64 %0) %1 = tail call i64 asm sideeffect "\0A vsetivli zero, 2, e64, m1, ta, ma\0A sf.vc.vv 0x3, 0x0, $1, $1;\0A ", "=r,^vr,~{memory},~{vl},~{sf_vcix_state}"(<vscale x 1 x i64> %vreg) ret i32 0 }

if we change sf_vcix_state to sf.vcix_state, the code doesn't work as expected, the instruction is still reordered, so I doubt that the name here is the defining name of the register in RISCVRegisterInfo.td rather than the actually name of the register.

I think here is the root cause:

llvm-project/llvm/include/llvm/CodeGen/TargetRegisterInfo.h

Lines 1106 to 1111 in 1e3a24d

// FIXME: We are assuming that the assembly name is equal to the TableGen

// name converted to lower case

//

// The TableGen name is the name of the definition for this register in the

// target's tablegen files. For example, the TableGen name of

// def EAX : Register <...>; is "EAX"

Maybe we should overwrite this function for RISCV?
What do you think @jrtc27 @kito-cheng ?

@jrtc27 @kito-cheng Friendly ping 🙂

@4vtomat we should override that function for this register.

Sure, let me add it!

topperc

LGTM

michalt · 2024-09-30T06:37:08Z

Thanks everyone! 😄 Is there anything still blocking us from merging this?

4vtomat · 2024-09-30T19:16:05Z

Thanks everyone! 😄 Is there anything still blocking us from merging this?

Actually no, let me merge it later today, thanks!

Resolved llvm#106700. This enables inline asm to have vcix_state to be a clobbered register thus disable reordering between VCIX intrinsics and inline asm.

michalt · 2024-10-01T07:04:34Z

Awesome, thank you! 😄

…m#106914) riscv-non-isa/riscv-toolchain-conventions#56 Resolved llvm#106700. This enables inline asm to have vcix_state to be a clobbered register thus disable reordering between VCIX intrinsics and inline asm.

llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 1, 2024

4vtomat requested review from topperc and wangpc-pp September 1, 2024 17:20

wangpc-pp reviewed Sep 2, 2024

View reviewed changes

4vtomat force-pushed the vcix_state_inline_asm branch from 5f821a2 to c8ae503 Compare September 3, 2024 12:34

kito-cheng reviewed Sep 5, 2024

View reviewed changes

4vtomat force-pushed the vcix_state_inline_asm branch from c8ae503 to 5370538 Compare September 23, 2024 07:11

topperc approved these changes Sep 23, 2024

View reviewed changes

4vtomat force-pushed the vcix_state_inline_asm branch from 5370538 to 87bbba7 Compare October 1, 2024 06:49

[RISCV][VCIX] Add vcix_state to GNU inline assembly register set

aff3876

Resolved llvm#106700. This enables inline asm to have vcix_state to be a clobbered register thus disable reordering between VCIX intrinsics and inline asm.

4vtomat force-pushed the vcix_state_inline_asm branch from 87bbba7 to aff3876 Compare October 1, 2024 06:52

4vtomat merged commit 23c0850 into llvm:main Oct 1, 2024
4 of 5 checks passed

4vtomat deleted the vcix_state_inline_asm branch October 1, 2024 06:52

		def VCIX_STATE : RISCVReg<0, "vcix_state">;
		def : RISCVRegisterClass<[XLenVT], 32, (add VCIX_STATE)> {

	// FIXME: We are assuming that the assembly name is equal to the TableGen
	// name converted to lower case
	//
	// The TableGen name is the name of the definition for this register in the
	// target's tablegen files. For example, the TableGen name of
	// def EAX : Register <...>; is "EAX"

[RISCV][VCIX] Add vcix_state to GNU inline assembly register set #106914

[RISCV][VCIX] Add vcix_state to GNU inline assembly register set #106914

Uh oh!

Conversation

4vtomat commented Sep 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Sep 1, 2024

Uh oh!

llvmbot commented Sep 1, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kito-cheng commented Sep 2, 2024

Uh oh!

4vtomat commented Sep 2, 2024

Uh oh!

kito-cheng commented Sep 2, 2024

Uh oh!

4vtomat commented Sep 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4vtomat Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4vtomat Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

michalt commented Sep 30, 2024

Uh oh!

4vtomat commented Sep 30, 2024

Uh oh!

Uh oh!

michalt commented Oct 1, 2024

Uh oh!

Uh oh!

4vtomat commented Sep 1, 2024 •

edited

Loading

4vtomat Sep 6, 2024 •

edited

Loading

4vtomat Sep 11, 2024 •

edited

Loading