Skip to content

[RISCV] Make All VType Parts Optional #144971

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 23, 2025
Merged

[RISCV] Make All VType Parts Optional #144971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c0edf6
[RISCV] Make All VType Parts Optional
lenary Jun 20, 2025
0eeb4db
clang-format
lenary Jun 20, 2025
6925bef
Add tests for skipped operands
lenary Jun 21, 2025
07ba0f9
Add out-of-order testcase
lenary Jun 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 65 additions & 64 deletions llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,14 @@ struct ParserOptionsSet {
};

class RISCVAsmParser : public MCTargetAsmParser {
// This tracks the parsing of the 4 operands that make up the vtype portion
// of vset(i)vli instructions which are separated by commas. The state names
// represent the next expected operand with Done meaning no other operands are
// expected.
enum VTypeState {
VTypeState_SEW,
VTypeState_LMUL,
VTypeState_TailPolicy,
VTypeState_MaskPolicy,
VTypeState_Done,
// This tracks the parsing of the 4 optional operands that make up the vtype
// portion of vset(i)vli instructions which are separated by commas.
enum class VTypeState {
SeenNothingYet,
SeenSew,
SeenLmul,
SeenTailPolicy,
SeenMaskPolicy,
};

SmallVector<FeatureBitset, 4> FeatureBitStack;
Expand Down Expand Up @@ -2233,25 +2231,30 @@ bool RISCVAsmParser::parseVTypeToken(const AsmToken &Tok, VTypeState &State,
return true;

StringRef Identifier = Tok.getIdentifier();

switch (State) {
case VTypeState_SEW:
if (!Identifier.consume_front("e"))
break;
if (State < VTypeState::SeenSew && Identifier.consume_front("e")) {
if (Identifier.getAsInteger(10, Sew))
break;
return true;
if (!RISCVVType::isValidSEW(Sew))
break;
State = VTypeState_LMUL;
return true;

State = VTypeState::SeenSew;
return false;
case VTypeState_LMUL: {
if (!Identifier.consume_front("m"))
break;
}

if (State < VTypeState::SeenLmul && Identifier.consume_front("m")) {
// Might arrive here if lmul and tail policy unspecified, if so we're
// parsing a MaskPolicy not an LMUL.
if (Identifier == "a" || Identifier == "u") {
MaskAgnostic = (Identifier == "a");
State = VTypeState::SeenMaskPolicy;
return false;
}

Fractional = Identifier.consume_front("f");
if (Identifier.getAsInteger(10, Lmul))
break;
return true;
if (!RISCVVType::isValidLMUL(Lmul, Fractional))
break;
return true;

if (Fractional) {
unsigned ELEN = STI->hasFeature(RISCV::FeatureStdExtZve64x) ? 64 : 32;
Expand All @@ -2262,30 +2265,32 @@ bool RISCVAsmParser::parseVTypeToken(const AsmToken &Tok, VTypeState &State,
Twine(MinLMUL) + " is reserved");
}

State = VTypeState_TailPolicy;
State = VTypeState::SeenLmul;
return false;
}
case VTypeState_TailPolicy:

if (State < VTypeState::SeenTailPolicy && Identifier.starts_with("t")) {
if (Identifier == "ta")
TailAgnostic = true;
else if (Identifier == "tu")
TailAgnostic = false;
else
break;
State = VTypeState_MaskPolicy;
return true;

State = VTypeState::SeenTailPolicy;
return false;
case VTypeState_MaskPolicy:
}

if (State < VTypeState::SeenMaskPolicy && Identifier.starts_with("m")) {
if (Identifier == "ma")
MaskAgnostic = true;
else if (Identifier == "mu")
MaskAgnostic = false;
else
break;
State = VTypeState_Done;
return true;

State = VTypeState::SeenMaskPolicy;
return false;
case VTypeState_Done:
// Extra token?
break;
}

return true;
Expand All @@ -2294,49 +2299,45 @@ bool RISCVAsmParser::parseVTypeToken(const AsmToken &Tok, VTypeState &State,
ParseStatus RISCVAsmParser::parseVTypeI(OperandVector &Operands) {
SMLoc S = getLoc();

unsigned Sew = 0;
unsigned Lmul = 0;
// Default values
unsigned Sew = 8;
unsigned Lmul = 1;
bool Fractional = false;
bool TailAgnostic = false;
bool MaskAgnostic = false;

VTypeState State = VTypeState_SEW;
SMLoc SEWLoc = S;

if (parseVTypeToken(getTok(), State, Sew, Lmul, Fractional, TailAgnostic,
MaskAgnostic))
return ParseStatus::NoMatch;

getLexer().Lex();

while (parseOptionalToken(AsmToken::Comma)) {
VTypeState State = VTypeState::SeenNothingYet;
do {
if (parseVTypeToken(getTok(), State, Sew, Lmul, Fractional, TailAgnostic,
MaskAgnostic))
MaskAgnostic)) {
// The first time, errors return NoMatch rather than Failure
if (State == VTypeState::SeenNothingYet)
return ParseStatus::NoMatch;
break;
}

getLexer().Lex();
}
} while (parseOptionalToken(AsmToken::Comma));

if (getLexer().is(AsmToken::EndOfStatement) && State == VTypeState_Done) {
RISCVVType::VLMUL VLMUL = RISCVVType::encodeLMUL(Lmul, Fractional);
if (Fractional) {
unsigned ELEN = STI->hasFeature(RISCV::FeatureStdExtZve64x) ? 64 : 32;
unsigned MaxSEW = ELEN / Lmul;
// If MaxSEW < 8, we should have printed warning about reserved LMUL.
if (MaxSEW >= 8 && Sew > MaxSEW)
Warning(SEWLoc,
"use of vtype encodings with SEW > " + Twine(MaxSEW) +
" and LMUL == mf" + Twine(Lmul) +
" may not be compatible with all RVV implementations");
}
if (!getLexer().is(AsmToken::EndOfStatement) ||
State == VTypeState::SeenNothingYet)
return generateVTypeError(S);

unsigned VTypeI =
RISCVVType::encodeVTYPE(VLMUL, Sew, TailAgnostic, MaskAgnostic);
Operands.push_back(RISCVOperand::createVType(VTypeI, S));
return ParseStatus::Success;
RISCVVType::VLMUL VLMUL = RISCVVType::encodeLMUL(Lmul, Fractional);
if (Fractional) {
unsigned ELEN = STI->hasFeature(RISCV::FeatureStdExtZve64x) ? 64 : 32;
unsigned MaxSEW = ELEN / Lmul;
// If MaxSEW < 8, we should have printed warning about reserved LMUL.
if (MaxSEW >= 8 && Sew > MaxSEW)
Warning(S, "use of vtype encodings with SEW > " + Twine(MaxSEW) +
" and LMUL == mf" + Twine(Lmul) +
" may not be compatible with all RVV implementations");
}

return generateVTypeError(S);
unsigned VTypeI =
RISCVVType::encodeVTYPE(VLMUL, Sew, TailAgnostic, MaskAgnostic);
Operands.push_back(RISCVOperand::createVType(VTypeI, S));
return ParseStatus::Success;
}

bool RISCVAsmParser::generateVTypeError(SMLoc ErrorLoc) {
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Target/RISCV/RISCVInstrInfoV.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ class VTypeIOp<int VTypeINum> : RISCVOp {
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isUInt<VTypeINum>(Imm);
return MCOp.isBareSymbolRef();
return isUInt<}] # VTypeINum # [{>(Imm);
return MCOp.Kind == KindTy::VType;
}];
}

Expand Down
27 changes: 4 additions & 23 deletions llvm/test/MC/RISCV/rvv/invalid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,6 @@
# RUN: not llvm-mc -triple=riscv64 --mattr=+v --mattr=+f %s 2>&1 \
# 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]

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]

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]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change the error message just like in #115277?

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly thought that trying to make it clear which bits are optional and which aren't with sort-of regex syntax would be more confusing than the current message. We can certainly look at improving them in the future, but probably by entirely revisiting how error messages are generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that we're doing this for compatibility with binutils, does it even make sense to encourage the optional syntax?

It's going to be rarely useful to not specify sew.
Not specifying tail and mask policy gives undisturbed policy which is the worst for performance on out of order cores.

Not specifying LMUL is only useful in loops with a single element type. If you do any widening/narrowing, you still need to change vtype to a different lmul to do any other operations on the data other than load/store. Having no lmul only on some vsetvli in your loop seems like bad readability choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want, I could look at immediately adding deprecation warnings when anything is left out?

That way it works correctly, but we also get to tell users we don't want to support the optional syntax indefinitely (presumably when another version of the spec is officially released/ratified, the fields will no longer be optional)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That way it works correctly, but we also get to tell users we don't want to support the optional syntax indefinitely (presumably when another version of the spec is officially released/ratified, the fields will no longer be optional)

What would be considered an official release? It's not listed as optional in the ISA manual from May 2025 here https://lf-riscv.atlassian.net/wiki/spaces/HOME/pages/16154769/RISC-V+Technical+Specifications#ISA-Specifications

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, sorry, that's maybe me speaking slightly inaccurately. It feels like this change needs some acknowledgement that it potentially breaks existing assembly rather than just updating examples. The change isn't really called out in the May 2025 release (but not many are), and the extension version isn't changed either, so it's really hard to tell if it was intended to break compatibility or not.

Anyway, I'm happy to add some kind of warning if we want to encourage users away from using the "everything is optional" form of these operands.

I think we did have consensus on the call that we should support the "everything is optional" form, so I implemented it.


Expand All @@ -22,10 +16,6 @@ vsetvli a2, a0, (1 << 11)
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]


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]

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]

Expand Down Expand Up @@ -59,12 +49,6 @@ vsetvli a2, a0, e8,m1,tx
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]

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]

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]

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]

Expand All @@ -80,17 +64,14 @@ vsetvli a2, a0, e8,m1,tu,mut
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]

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]

vsetvli a2, a0, e8,m1
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]

vsetvli a2, a0, e8,m1,ta
vsetvli a2, a0, ma,tu,m1,e8
# 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]
vsetvli a2, a0,
# CHECK-ERROR: unknown operand

vadd.vv v1, v3, v2, v4.t
# CHECK-ERROR: operand must be v0.t
Expand Down
Loading