[RISCV] Assemble/Disassemble v-ext instructions.

[HsiangKai]

Summary

Assemble/disassemble RISC-V V extension instructions according to version 0.8 in https://github.com/riscv/riscv-v-spec/.

I have tested this patch using GNU toolchain. The encoding is aligned to GNU assembler output. In this patch, there is a test case for each instruction at least.

The V register definition is just for assemble/disassemble. Its type is not important in this stage. I think it will be reviewed and modified as we want to do codegen for scalable vector types.

This patch does not include Zvqmac, Zvamo, Zvlsseg, and Zvediv.

[HsiangKai]

Update test cases.

[fpallares]

llvm/test/MC/RISCV/rvv/invalid.s

6

Suggestion: If you use CHECK-LABEL between the regular CHECK you can avoid these directives to succeed by matching lines corresponding to a later instruction. Like this:

​vsetvli a2, a0, e31
// CHECK-ERROR: operand must be e[8|16|32|64|128|256|512|1024],m[1|2|4|8]
// CHECK-ERROR-LABEL: ​vsetvli a2, a0, e31

(Note the CHECK-LABEL is placed after the CHECK since it appears in this order in the output)

Without the CHECK-LABELS the test will still fail when something is wrong, but with them FileCheck will be able to point exactly where the problem occurred.

More info on this in the corresponding section of the FileCheck manual.

[HsiangKai]

llvm/test/MC/RISCV/rvv/invalid.s

6

Got it. Thanks.

[HsiangKai]

In RISCVRegisterInfo.td, set offset of vector sub-registers to -1.

[HsiangKai]

• Rewrite validateInstruction() in RISCVAsmParser.cpp. Use TSFlags to classify instructions' constraints.
• Update invalid.s test cases.

[HsiangKai]

• Rebase on master branch.
• Fix validInstruction() bugs.
• Update test cases.

[HsiangKai]

• Fix clang-format errors.
• Rename variables according to LLVM Coding Standards.

[HsiangKai]

• Rebase master.
• Make "V" extension as an experimental one.
  • This follows the design as discussed on the mailing lists in the following RFC: http://lists.llvm.org/pipermail/llvm-dev/2020-January/138364.html

[simoncook]

This is looking good, overall the patch is nicely laid out which has made it easy to compare against the spec.

I've made a few comments, mostly about ordering of instructions so that they are identical to the spec.

The other thing I've noticed looking through the spec, there are references to psuedoinstructions, but these aren't covered by this patch. It would be good for InstAliases to be defined for these and land in the same patch.

Edit: I did have some question about Zvediv/Zvqmac being missing, but I notice now that's intentional as per the summary

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

160

If you are going to rely on the register names being contiguous, extend the static_asserts in RISCVRegisterInfo.cpp to validate this at compile time.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

450

Add comment noting end of earlyclobber/constraint scope

533

I'm not sure if this will read better if you move the common let vs2=0, vm=1 up here?

552

Add // hasSideEffects = 0, mayLoad = 0, mayStore = 0

579

Following through the 0.8 spec I would have expected the defm VFADD_V : VALU_FV_V_F result here, since the previous instruction was defined in section 13.5, and VFADD.V[VF] is the next thing defined in 14.2.

The chapter 14 instructions look like they are defined in the order the spec says (with the exception of VFREDOSUM, etc at the bottom of this file). Could you move chapter 13 instructions here so the order matches, to make it easier to validate as the spec advances.

649

The vfmv definition from line 814 should move here? (17.2?)

652

This should be below vcompress?

[HsiangKai]

• Address @simoncook's comments.
• Reorder instructions according to "V" specification.
• Add pseudo instructions in "V" specification.
• Add comments in RISCVInstrInfoV.td
• "V" implies "F".

[HsiangKai]

• Refine naming.
• Some typo.

[HsiangKai]

Rebase on master.

[HsiangKai]

Fix clang-tidy warnings.

[HsiangKai]

Is this patch ready to land? Are there any comments or suggestions I missed?

[rogfer01]

@HsiangKai: just to confirm and to avoid confusion for other reviewers.

Assemble/disassemble RISC-V V extension instructions according to version 0.8-draft-20191004 in https://github.com/riscv/riscv-v-spec/.

Is the patch against the spec published in https://github.com/riscv/riscv-v-spec/releases/tag/0.8 (which looks to me from 20191214)?

[HsiangKai]

In D69987#1853986, @HsiangKai wrote:

Update to version 0.8-draft-20191213.

Hi Roger,

I have updated it to 0.8-draft-20191213 in February. It is the same as version 0.8. Sorry for that I did not update the commit message. Thanks for your kindly reminding.

[fpallares]

This comment was removed by fpallares.

[fpallares]

Hi Kai, I've added an inline comment regarding constraint validation:

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

2290

In this function we are validating the overlap constraints by checking whether DestReg matches a vector operand. However, for widenings and narrowings those checks should be extended to validate that the operand doesn't belong to the same register group.

For instance:

vwadd.vv v4, v4, v7: The current code would give an error for this instruction.
vwadd.vv v4, v5, v7: There would be no error for this, even though v5 will always overlap with any widened group of v4.
vwadd.vv v4, v6, v7: This should not emit any diagnostic because we can't really tell at this point if there will be an overlap.

Perhaps we are already checking this somewhere else and I missed it?

(Note that this will probably change with fractional LMUL in 0.9)

[HsiangKai]

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

2290

Good point!

Because there is no LMUL information in these instructions, we cannot detect the invalid cases precisely. I only detect destination and source occupy the same register. Maybe I could detect destination and (source + 1). The smallest LMUL is 1 in v0.8.

After this patch, I am going to prepare another one to upgrade the MC layer to v0.9. I will review this part.

Do you have any suggestions?

[fpallares]

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

2290

Yes, I was thinking on the (source + 1) case too. I don't think much can be done in v0.9 at the MC layer, since this case would be correct for LMUL < 1.

[fpallares]

I've added a couple more inline comments.

I've also noticed that we aren't emitting the aliases for unmasked instructions, for instance:

(input → output)

vnot.v v8, v4, v0.t → vnot.v v8, v4, v0.t
​vnot.v v8, v4       → vxor.vi v8, v4, -1

AFAICT RISC-V doesn't define a preferred way to disassemble, so I'd say this is something minor.
After looking a bit into it I believe this is caused by MCInstPrinter not expecting the VMaskOp operand to be set to NoRegister, and failing to match the alias pattern. We have a downstream patch that makes this case work, though it's a bit hackish.

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

1080

We are missing a case for the newly added InvalidSImm5Plus1 here.

Perhaps something in the lines of:

case Match_InvalidSImm5Plus1:                                                 
  return generateImmOutOfRangeError(                                          
      Operands, ErrorInfo, -(1 << 4) + 1, (1 << 4),                           
      "immediate must be in the range");

This can be tested using the following input:

vmslt.vi v1, v2, -16                                                            
vmslt.vi v1, v2, 17

(we would expect the diagnostic error: immediate must be in the range [-15, 16])

llvm/lib/Target/RISCV/RISCVRegisterInfo.td

296

I think this definition is no longer used in the latest version of the patch.

llvm/test/MC/RISCV/rvv/add.s

325

Should this be # CHECK-INST: vwadd.vx v8, v4, zero?

[HsiangKai]

• Check operands for widening and narrowing instructions.
• Modify according to @fpallares's comments.

[HsiangKai]

Apply clang-format.

[HsiangKai]

Ping.

[MaskRay]

Drive-by comment: the clang side change isn't tightly coupled with the LLVM side changes. It should be a separate patch.

llvm/test/MC/RISCV/rvv/add.s

2

< %s -> %s

ditto for every other test.

A check prefix does not have to start with CHECK-.
If shortening the prefix can avoid wrapped lines, probably worth considering it.

[HsiangKai]

In D69987#2074265, @MaskRay wrote:

Drive-by comment: the clang side change isn't tightly coupled with the LLVM side changes. It should be a separate patch.

Create https://reviews.llvm.org/D81188 for clang side change.

[evandro]

It looks pretty GTM. At this point, I'd be fine with accepting this patch as the major issues seem to have already been addressed. Should there be any other minor issue, it could be addressed later.

[evandro]

llvm/lib/Target/RISCV/RISCVInstrFormats.td

56

Methinks that these constraints WidenV, WidenW, WidenCvt, should be split up by their components. IOW, into Widen, Wide (input), Cvt. This way, it's easier to test for specific constraints.

61

Likewise, this constraint could then be removed, but Narrow && Cvt would achieve the same meaning.

[HsiangKai]

llvm/lib/Target/RISCV/RISCVInstrFormats.td

56

Do you mean

WidenV = Widen;
WidenW = Widen | WideInput;
WidenCvt = Widen | Cvt;

[HsiangKai]

Remove redundant RVVConstraint.

[rogfer01]

The patch as it stands now LGTM and I think it can be committed. Is there any objection remaining?

Any further comments @simoncook @asb?

[fpallares]

I'm not a reviewer but the patch LGTM, thanks for all the changes @HsiangKai.

[evandro]

llvm/lib/Target/RISCV/RISCVInstrFormats.td

56

Yes.

[asb]

In D69987#2079524, @rogfer01 wrote:

The patch as it stands now LGTM and I think it can be committed. Is there any objection remaining?

Any further comments @simoncook @asb?

This LGTM - thanks @HsiangKai and everyone who's been working on this. As Simon said before, it's really nicely laid out which makes it much easier to review.

[asb]

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

10

Please add similar language as in RISCVInstrInfoB.td to indicate the version being described and explain this version is experimental and hasn't been ratified.

[HsiangKai]

llvm/lib/Target/RISCV/RISCVInstrFormats.td

56

Got it. I will improve it based on v0.9 implementation.

[HsiangKai]

llvm/lib/Target/RISCV/RISCVInstrInfoV.td

10

Got it. I will add more comments similar to RISCVInstrInfoB.td before landing the patch.

[evandro]

llvm/lib/Target/RISCV/RISCVInstrFormats.td

56

Please, address this comment before committing.