Skip to content

Conversation

@saveasguy
Copy link

@saveasguy saveasguy commented Nov 7, 2024

From RISC-V Unprivileged Specification version 20240411 (https://github.com/riscv/riscv-isa-manual/releases/tag/20240411) , paragraph 31.6, it's valid to omit LMUL parameter.
GCC has already supported it: https://godbolt.org/z/qs1svYY44.
So this patch introduces support of such behavior.

@github-actions
Copy link

github-actions bot commented Nov 7, 2024

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot llvmbot added backend:RISC-V llvm:mc Machine (object) code labels Nov 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-mc

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

Author: Aleksei Romanov (saveasguy)

Changes

From RISC-V Unprivileged Specification version 20240411 (https://github.com/riscv/riscv-isa-manual/releases/tag/20240411) , paragraph 31.6, it's valid to omit LMUL parameter.
GCC has already supported it: https://godbolt.org/z/YxnEK3aer.
So this patch introduces support of such behavior.


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

3 Files Affected:

  • (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+9-3)
  • (modified) llvm/test/MC/RISCV/rvv/invalid.s (+30-30)
  • (modified) llvm/test/MC/RISCV/rvv/vsetvl.s (+7)
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 4d46afb8c4ef97..77d527c4fafe3f 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2222,8 +2222,14 @@ bool RISCVAsmParser::parseVTypeToken(const AsmToken &Tok, VTypeState &State,
     State = VTypeState_LMUL;
     return false;
   case VTypeState_LMUL: {
-    if (!Identifier.consume_front("m"))
-      break;
+    // Set LMUL to default if it is omitted.
+    if (!Identifier.consume_front("m")) {
+      Lmul = 1;
+      Fractional = false;
+      State = VTypeState_TailPolicy;
+      return parseVTypeToken(Tok, State, Sew, Lmul, Fractional, TailAgnostic,
+                             MaskAgnostic);
+    }
     Fractional = Identifier.consume_front("f");
     if (Identifier.getAsInteger(10, Lmul))
       break;
@@ -2320,7 +2326,7 @@ bool RISCVAsmParser::generateVTypeError(SMLoc ErrorLoc) {
   return Error(
       ErrorLoc,
       "operand must be "
-      "e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]");
+      "e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}");
 }
 
 ParseStatus RISCVAsmParser::parseMaskReg(OperandVector &Operands) {
diff --git a/llvm/test/MC/RISCV/rvv/invalid.s b/llvm/test/MC/RISCV/rvv/invalid.s
index 07e7b9db6606c5..5bee95f4a1f1c7 100644
--- a/llvm/test/MC/RISCV/rvv/invalid.s
+++ b/llvm/test/MC/RISCV/rvv/invalid.s
@@ -2,95 +2,95 @@
 # RUN:        | FileCheck %s --check-prefix=CHECK-ERROR
 
 vsetivli a2, 32, e8,m1
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetivli a2, zero, e8,m1
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetivli a2, 5, (1 << 10)
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetivli a2, 5, 0x400
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetivli a2, 5, e31
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, (1 << 11)
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, 0x800
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 
 vsetvli a2, a0, e31
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e32,m3
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, m1,e32
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e32,m16
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e128,m8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e256,m8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e512,m8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e1024,m8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e2048,m8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e1,m8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,tx
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,ta,mx
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,ma
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,mu
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8x,m1,tu,mu
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1z,tu,mu
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,mf1,tu,mu
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,tu,mut
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,tut,mu
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,m1,ta
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vsetvli a2, a0, e8,1,ta,ma
-# CHECK-ERROR: operand must be e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]
+# CHECK-ERROR: operand must be e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}
 
 vadd.vv v1, v3, v2, v4.t
 # CHECK-ERROR: operand must be v0.t
diff --git a/llvm/test/MC/RISCV/rvv/vsetvl.s b/llvm/test/MC/RISCV/rvv/vsetvl.s
index 2741def0eeff21..37e763c24cde15 100644
--- a/llvm/test/MC/RISCV/rvv/vsetvl.s
+++ b/llvm/test/MC/RISCV/rvv/vsetvl.s
@@ -10,6 +10,13 @@
 # RUN: llvm-mc -triple=riscv64 -filetype=obj --mattr=+v %s \
 # RUN:        | llvm-objdump -d - | FileCheck %s --check-prefix=CHECK-UNKNOWN
 
+# LMUL is omitted.
+vsetvli a2, a0, e8, ta, ma
+# CHECK-INST: vsetvli a2, a0, e8, m1, ta, ma
+# CHECK-ENCODING: [0x57,0x76,0x05,0x0c]
+# CHECK-ERROR: instruction requires the following: 'V' (Vector Extension for Application Processors), 'Zve32x' (Vector Extensions for Embedded Processors){{$}}
+# CHECK-UNKNOWN: 0c057657 <unknown>
+
 # reserved filed: vlmul[2:0]=4, vsew[2:0]=0b1xx, non-zero bits 8/9/10.
 vsetvli a2, a0, 0x224
 # CHECK-INST: vsetvli a2, a0, 548

ErrorLoc,
"operand must be "
"e[8|16|32|64],m[1|2|4|8|f2|f4|f8],[ta|tu],[ma|mu]");
"e{8|16|32|64},[m{1|2|4|8|f2|f4|f8},]{ta|tu},{ma|mu}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change [ to {? Mask/tail policies are optional I think. But the original format is not right, it should be e{8|16|32|64}[,m{1|2|4|8|f2|f4|f8}][,{ta|tu}][,{ma|mu}] now?

Copy link
Author

@saveasguy saveasguy Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp#L1651 -- here [ means optional so you're right that original format is not ok. This is why I've changed [ to {.
  2. As far as I can see from RISC-V spec mask/tail policies are not optional. GCC also doesn't think so: https://godbolt.org/z/54h1Kf3a1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually GCC(binutils) supports optional mask/tail policies: https://godbolt.org/z/GPEG8xzrf
It's a known issue for LLVM if I remember correctly, we may have some discussion before but I can't find the link.
Binutils supports such thing just for compatibility with old version (0.7.1, pity) of RVV.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, please use e{8|16|32|64}[,m{1|2|4|8|f2|f4|f8}],{ta|tu},{ma|mu} (put [ in front of ,) in case we may support optional policies someday.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, it's better to stick with the current changes since looks like RVV 1.0 doesn't support optional mask/tail policies, and someone who is interested in compatibility with the RVV 0.7.1 can do changes above this PR.
Anyway, I would like to hear your opinion on this.

Copy link
Contributor

@wangpc-pp wangpc-pp Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, please use e{8|16|32|64}[,m{1|2|4|8|f2|f4|f8}],{ta|tu},{ma|mu} (put [ in front of ,) in case we may support optional policies someday.

This just suggests you to change the position of [/], it won't touch other parts. The effect of the regex is still the same.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed.

@saveasguy saveasguy force-pushed the allow-vsetvli-omitting-lmul branch from 61f51b2 to fcb0b72 Compare November 7, 2024 12:06
@saveasguy saveasguy changed the title [RISCV][AsmParser] Allow parsing vsetvl omitting LMUL parameter [RISCV][AsmParser] Allow parsing vset{i}vli omitting LMUL parameter Nov 7, 2024
@saveasguy saveasguy force-pushed the allow-vsetvli-omitting-lmul branch from fcb0b72 to 305fe6a Compare November 7, 2024 13:08
@saveasguy saveasguy force-pushed the allow-vsetvli-omitting-lmul branch from 305fe6a to fa5db8e Compare November 7, 2024 13:11
This enables support of vset{i}vli instructions omitting LMUL
corresponding to RISC-V specification.
@saveasguy saveasguy force-pushed the allow-vsetvli-omitting-lmul branch from fa5db8e to d027839 Compare November 7, 2024 13:15
@topperc
Copy link
Collaborator

topperc commented Nov 7, 2024

The spec was changed to make all fields required here riscv/riscv-isa-manual#1489

@topperc
Copy link
Collaborator

topperc commented Nov 7, 2024

The godbolt link in the description gives an error rather than demonstrating anything <source>:3: Error: unrecognized opcode vsetvli a2,a0,e8,ta,ma', extension v' or zve64x' or zve32x' required

@saveasguy
Copy link
Author

The godbolt link in the description gives an error rather than demonstrating anything <source>:3: Error: unrecognized opcode vsetvli a2,a0,e8,ta,ma', extension v' or zve64x' or zve32x' required

Sorry, fixed it. Anyway, do we want to proceed with this patch?

@saveasguy
Copy link
Author

@topperc I'm sorry to bother you, could you address the last comment, please?

@topperc
Copy link
Collaborator

topperc commented Nov 13, 2024

@topperc I'm sorry to bother you, could you address the last comment, please?

I'm not sure we should support it since the spec no longer indicates it is optional.

As far as I know all fields are optional in binutils so if we're just doing it for compatibility, why only make 1 field optional?

@saveasguy
Copy link
Author

Ok, so I close this PR.

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

Labels

backend:RISC-V llvm:mc Machine (object) code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants