Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inline assembly needs to accept RISC-V Vector vl register as clobberable for GCC compatibility #67596

Closed
sh1boot opened this issue Sep 27, 2023 · 4 comments · Fixed by #67646
Closed

Comments

@sh1boot
Copy link

sh1boot commented Sep 27, 2023

GCC for RISC-V supports a vl register in its clobber list, so it knows when the assembly contains vsetvli or similar. Clang seems to assume it's clobbered, but since it doesn't accept the name in the clobber list it's not source-level compatible.

https://godbolt.org/z/Ecf3q64qP

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 28, 2023

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

GCC for RISC-V supports a `vl` register in its clobber list, so it knows when the assembly contains `vsetvli` or similar. Clang seems to assume it's clobbered, but since it doesn't accept the name in the clobber list it's not source-level compatible.

https://godbolt.org/z/Ecf3q64qP

@wangpc-pp
Copy link
Contributor

This can be supported by adding vl to RISCVTargetInfo::getGCCRegNames():

diff --git a/clang/lib/Basic/Targets/RISCV.cpp b/clang/lib/Basic/Targets/RISCV.cpp                                                                                                                                               
index d55ab76395c8..e2ed89074630 100644
--- a/clang/lib/Basic/Targets/RISCV.cpp
+++ b/clang/lib/Basic/Targets/RISCV.cpp
@@ -40,7 +40,10 @@ ArrayRef<const char *> RISCVTargetInfo::getGCCRegNames() const {
       "v0",  "v1",  "v2",  "v3",  "v4",  "v5",  "v6",  "v7",
       "v8",  "v9",  "v10", "v11", "v12", "v13", "v14", "v15",
       "v16", "v17", "v18", "v19", "v20", "v21", "v22", "v23",
-      "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31"};
+      "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31",
+
+      "vl"
+      };
   return llvm::ArrayRef(GCCRegNames);
 }

If so, should we add all registers? @kito-cheng @asb

@sh1boot
Copy link
Author

sh1boot commented Oct 4, 2023

I'm not sure which registers are assume-clobbered in Clang's inline assembly. When you suggest "all registers", is there a list of always-clobbereds which could be silently accepted in this context?

@wangpc-pp
Copy link
Contributor

I'm not sure which registers are assume-clobbered in Clang's inline assembly. When you suggest "all registers", is there a list of always-clobbereds which could be silently accepted in this context?

I don't have a list (and I don't think there is a spec that describes these registers).
For GCC, all registers in REGISTER_NAMES (gcc/config/riscv/riscv.h) can be accepted in clobberred list:

#define REGISTER_NAMES						\
{ "zero","ra",  "sp",  "gp",  "tp",  "t0",  "t1",  "t2",	\
  "s0",  "s1",  "a0",  "a1",  "a2",  "a3",  "a4",  "a5",	\
  "a6",  "a7",  "s2",  "s3",  "s4",  "s5",  "s6",  "s7",	\
  "s8",  "s9",  "s10", "s11", "t3",  "t4",  "t5",  "t6",	\
  "ft0", "ft1", "ft2", "ft3", "ft4", "ft5", "ft6", "ft7",	\
  "fs0", "fs1", "fa0", "fa1", "fa2", "fa3", "fa4", "fa5",	\
  "fa6", "fa7", "fs2", "fs3", "fs4", "fs5", "fs6", "fs7",	\
  "fs8", "fs9", "fs10","fs11","ft8", "ft9", "ft10","ft11",	\
  "arg", "frame", "vl", "vtype", "vxrm", "frm", "N/A", "N/A",   \
  "N/A", "N/A", "N/A", "N/A", "N/A", "N/A", "N/A", "N/A",	\
  "N/A", "N/A", "N/A", "N/A", "N/A", "N/A", "N/A", "N/A",	\
  "N/A", "N/A", "N/A", "N/A", "N/A", "N/A", "N/A", "N/A",	\
  "v0",  "v1",  "v2",  "v3",  "v4",  "v5",  "v6",  "v7",	\
  "v8",  "v9",  "v10", "v11", "v12", "v13", "v14", "v15",	\
  "v16", "v17", "v18", "v19", "v20", "v21", "v22", "v23",	\
  "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31",}

For LLVM, only registers defined in getGCCRegNames(clang/lib/Basic/Targets/RISCV.cpp) will be accepted:

ArrayRef<const char *> RISCVTargetInfo::getGCCRegNames() const {
  static const char *const GCCRegNames[] = {
      // Integer registers
      "x0",  "x1",  "x2",  "x3",  "x4",  "x5",  "x6",  "x7",
      "x8",  "x9",  "x10", "x11", "x12", "x13", "x14", "x15",
      "x16", "x17", "x18", "x19", "x20", "x21", "x22", "x23",
      "x24", "x25", "x26", "x27", "x28", "x29", "x30", "x31",
      // Floating point registers
      "f0",  "f1",  "f2",  "f3",  "f4",  "f5",  "f6",  "f7",
      "f8",  "f9",  "f10", "f11", "f12", "f13", "f14", "f15",
      "f16", "f17", "f18", "f19", "f20", "f21", "f22", "f23",
      "f24", "f25", "f26", "f27", "f28", "f29", "f30", "f31",
      // Vector registers
      "v0",  "v1",  "v2",  "v3",  "v4",  "v5",  "v6",  "v7",
      "v8",  "v9",  "v10", "v11", "v12", "v13", "v14", "v15",
      "v16", "v17", "v18", "v19", "v20", "v21", "v22", "v23",
      "v24", "v25", "v26", "v27", "v28", "v29", "v30", "v31"};
  return llvm::ArrayRef(GCCRegNames);
}

There are some mismatches between GCC and LLVM (ignoring the register aliases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants