Upgrade MC implementation to V-extension v0.9.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Hi @HsiangKai, thanks for the patch. So far everything looks good aside from a couple of minor nits.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
1634 | While the spec currently states that specifying the vta and mta in vsetvli is not mandatory, it also states that not specifying them should be deprecated. Therefore, I'm not sure we should allow specifying the vta without specifying the mta. | |
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp | ||
172 | Given that the use of vsetvli without these flags should be deprecated, I'd recommend printing them always, even if it more verbose. |
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
796 | Minor nit that we missed in the patch of 0.8: can you replace this return with llvm_unreachable(); as recommended in the second paragraph of https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations Ditto in getSEWStr above. |
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
2381 | Given that this constraint has no effect when DestReg != RISCV::V0, we could simplify the logic by adding this to the condition: if (TargetFlags & RISCV::VMConstraint && DestReg == RISCV::V0) { Then the DestReg checks within the block can go away. | |
2381 | With the suggestion above, this could be further simplified to: if ((TargetFlags & RISCV::OneInput && Inst.getNumOperands() == 3) || Inst.getNumOperands() == 4) return Error(Loc, "The destination vector register group cannot overlap" " the mask register."); |
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
2386 | I think we might not need to treat vadc and vsbc specially here. Since those have 4 operands they should fall on the else branch of the next if-else block: if (Inst.getNumOperands() == 4) CheckReg = Inst.getOperand(3).getReg(); And that should produce the expected result. Did I miss some other reason why those should be treated separately? |
I've gone through and can't see any obvious issues. I defer to one of the RISC-V Vector extension usual suspects for giving a LGTM on the detail of the altered instructions etc. Once we have that, this looks good to land IMHO.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
2381 | I see you have updated the patch and removed the if that checked whether we were dealing with the masked versions of the instructions (by checking the number of operands). IIUC that check is still necessary so we don't report an error for unmasked instruction. For example, from my understanding the following instruction is correct, but we will be reporting an error. viota.m v0, v2 |
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
2381 | Good catch. Thank you. |
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
98–99 | I believe that with the changes introduced in the encoding of the loads and stores we can do without the mop parameter in most of the classes here:
We still need to keep the parameter for the VIndexedStore class since it can take MOPSTIndexedOrder (i.e. 11) or MOPSTIndexedUnord (i.e. 01). Does this make sense to you? |
To simplify the instruction definitions for vector load/store as @fpallares' suggestions.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
98–99 | It makes sense. Thanks. |
Since this patch replaces 0.8 support with 0.9, it should include an update to the version check in clang/lib/Driver/ToolChains/Arch/RISCV.cpp to match.
Apologies we didn't identify this earlier but with the change of the mask register layout (MLEN=1) the overlap constraints involving the mask register were modified:
RVV-0.8, Section 5.3. Vector Masking:
The destination vector register group for a masked vector instruction can only overlap the source mask register (v0) when LMUL=1. Otherwise, an illegal instruction exception is raised.
RVV-0.9, Section 5.3. Vector Masking:
The destination vector register group for a masked vector instruction cannot overlap the source mask register (v0), unless the destination vector register is being written with a mask value (e.g., comparisons) or the scalar result of a reduction. Otherwise, an illegal instruction exception is raised.
The change was introduced in this commit.
From my understanding, with this change an instruction such as the following should be rejected in RVV-0.9:
vadd.vv v0, v1, v2, v0.t
Also note that vadc/vsbc already have this behaviour.
Indeed, I did not take care of the mask register constraint for instructions. I will handle it in the next revision.
According the description from v0.9,
"The destination vector register group for a masked vector instruction cannot overlap the source mask register (v0), unless the destination vector register is being written with a mask value (e.g., comparisons) or the scalar result of a reduction. Otherwise, an illegal instruction exception is raised."
Check destination vector register group with v0 by default.
Thanks for the update @HsiangKai.
I've noticed that we aren't handling the exceptions that state that the V0 constraint shouldn't be enforced for instructions that generate masks or for reductions.
For instance the following (valid) instructions are rejected:
vmslt.vv v0, v2, v3, v0.t
vredsum.vs v0, v1, v2, v0.t
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | ||
---|---|---|
776 | I believe the LLVM Coding Standards recommend against default labels in switches over enums:
|
Aside from the minor nit below this patch LGTM. Thanks a lot for all the changes @HsiangKai.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td | ||
---|---|---|
542 | Minor nit: Add comment here (for the other let RVVConstraint = NoConstraint blocks below as well). |
This and the next value don't need to be specified.