Page MenuHomePhabricator

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

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



Upgrade MC implementation to V-extension v0.9.

Diff Detail

Unit TestsFailed

10,670 mslinux > libomp.env::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && env KMP_DISP_NUM_BUFFERS=0 /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp
1,340 mslinux > libomp.worksharing/for::kmp_set_dispatch_buf.c
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

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

Bad indentation


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.


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.


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.


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

Minor nit that we missed in the patch of 0.8: can you replace this return with


as recommended in the second paragraph of

Ditto in getSEWStr above.

fpallares added inline comments.Wed, Jul 1, 4:53 AM

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.


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

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.