Page MenuHomePhabricator

[RISCV] Upgrade RVV MC to v0.9.
ClosedPublic

Authored by HsiangKai on May 29 2020, 7:43 AM.

Details

Summary

Upgrade MC implementation to V-extension v0.9.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
MaskRay added inline comments.May 29 2020, 9:29 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1216–1219

Bad indentation

1613

Bad indentation

HsiangKai updated this revision to Diff 267447.May 30 2020, 2:11 AM
HsiangKai updated this revision to Diff 267448.May 30 2020, 2:40 AM

Again, the clang part should be split in another patch and be made a child of D81188.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
297

This and the next value don't need to be specified.

HsiangKai updated this revision to Diff 268644.Jun 4 2020, 7:42 PM

Address @evandro's comments.

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.

HsiangKai updated this revision to Diff 273397.Jun 25 2020, 9:05 AM

Address @fpallares's comments.

HsiangKai updated this revision to Diff 273406.Jun 25 2020, 9:10 AM

Simplify the logic of validateInstruction() in RISCVAsmParser.cpp.

HsiangKai updated this revision to Diff 274310.Jun 29 2020, 7:35 PM

Rebase on master.

Correct instructions validation according to v0.9 spec.

Update patch with full context.

rogfer01 added inline comments.Jul 1 2020, 1:19 AM
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.

fpallares added inline comments.Jul 1 2020, 4:53 AM
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.");
fpallares added inline comments.Jul 1 2020, 6:25 AM
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?

HsiangKai updated this revision to Diff 276005.Jul 7 2020, 5:09 AM

Address @rogfer01 and @fpallares' comments.

asb added a comment.Jul 8 2020, 10:19 PM

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.

fpallares added inline comments.Jul 13 2020, 9:17 AM
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
HsiangKai marked an inline comment as done.Jul 14 2020, 7:49 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2381

Good catch. Thank you.

HsiangKai updated this revision to Diff 278107.Jul 15 2020, 1:45 AM

Refine RISCVAsmParser::validateInstruction().

HsiangKai updated this revision to Diff 278108.Jul 15 2020, 1:48 AM

clang format.

fpallares added inline comments.Jul 15 2020, 10:08 AM
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:

classreplace mop by
VUnitStrideLoadMOPLDUnitStride (i.e. 00)
VStridedLoadMOPLDStrided (i.e. 10)
VIndexedLoadMOPLDIndexed (i.e. 11)
VUnitStrideStoreMOPSTUnitStride (i.e. 00)
VStridedStoreMOPLDStrided (i.e. 10)

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?

HsiangKai updated this revision to Diff 278393.Jul 16 2020, 1:29 AM

To simplify the instruction definitions for vector load/store as @fpallares' suggestions.

HsiangKai updated this revision to Diff 278397.Jul 16 2020, 1:42 AM

Simplify instruction format for vector load/store.

HsiangKai marked an inline comment as done.Jul 16 2020, 2:10 AM
HsiangKai added inline comments.
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.

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.

The modification is put in D81213.

The modification is put in D81213.

Ah ok, missed that, thanks for pointing it out

Just a couple of nits, but otherwise it LGTM.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
776

I'd rather the first case be the default case and that it would call llvm_unreachabe() instead.

796

Ditto.

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.

HsiangKai updated this revision to Diff 279443.Jul 21 2020, 1:52 AM

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:

-Wswitch warns if a switch, without a default label, over an enumeration does not cover every enumeration value. If you write a default label on a fully covered switch over an enumeration then the -Wswitch warning won’t fire when new elements are added to that enumeration.

See https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations .

Address @fpallares's comments.

fpallares accepted this revision.Jul 28 2020, 1:48 AM

Aside from the minor nit below this patch LGTM. Thanks a lot for all the changes @HsiangKai.

This revision is now accepted and ready to land.Jul 28 2020, 1:48 AM
fpallares added inline comments.Jul 28 2020, 1:51 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
542

Minor nit: Add comment here (for the other let RVVConstraint = NoConstraint blocks below as well).

HsiangKai updated this revision to Diff 281158.Jul 28 2020, 2:42 AM

Add comments in RISCVInstrInfoV.td.

Thanks for your review, @fpallares.

evandro accepted this revision.Jul 28 2020, 3:19 PM
This revision was landed with ongoing or failed builds.Jul 31 2020, 4:42 PM
Closed by commit rG47a4a27f4720: Upgrade MC to v0.9. (authored by HsiangKai). · Explain Why
This revision was automatically updated to reflect the committed changes.