Page MenuHomePhabricator

[RISCV] Upgrade RVV MC to v0.9.
Needs ReviewPublic

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

Details

Summary

Upgrade MC implementation to V-extension v0.9.

Diff Detail

Event Timeline

HsiangKai created this revision.May 29 2020, 7:43 AM
HsiangKai updated this revision to Diff 267239.May 29 2020, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 7:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay added inline comments.May 29 2020, 9:29 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1213–1216

Bad indentation

1617

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
1638

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.Thu, Jun 25, 9:05 AM

Address @fpallares's comments.

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

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

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

Rebase on master.

Correct instructions validation according to v0.9 spec.

Update patch with full context.

rogfer01 added inline comments.Wed, Jul 1, 1:19 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
794–795

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.Wed, Jul 1, 4:53 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2397

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.

2403

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.Wed, Jul 1, 6:25 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2402

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.Tue, Jul 7, 5:09 AM

Address @rogfer01 and @fpallares' comments.

asb added a comment.Wed, Jul 8, 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.