Page MenuHomePhabricator

[RISCV] Assemble/Disassemble v-ext instructions.
Needs ReviewPublic

Authored by HsiangKai on Nov 7 2019, 10:43 PM.

Details

Summary

Assemble/disassemble RISC-V V extension instructions according to version 0.8-draft-20191004 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 Zvamo, Zvlsseg, and Zvediv.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
HsiangKai added inline comments.Nov 18 2019, 12:05 AM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2034

Operands[0] in OperandVector will be operator. Operands[1] will be the first operand.
If there is an instruction with no operand, it might be an out-of-bounds access.

I will change the default location to point to operator. Thanks.

HsiangKai updated this revision to Diff 229964.Nov 18 2019, 7:50 PM

I create a branch "rvv" in this github repo https://github.com/sifive/riscv-llvm/tree/rvv.
I will track the v-extension spec in this repo.

This commit will also be maintained if there are any comments.
I will also update this commit if there are major updates in v-ext spec.

Joy12138 removed a subscriber: Joy12138.Jan 3 2020, 2:07 AM
Joy12138 added a subscriber: Joy12138.

FYI, the implementation of Zvamo, Zvlsseg, Zvediv and Pseudo instructions is available here: https://github.com/isrc-cas/rvv-llvm

evandro added a subscriber: evandro.Jan 9 2020, 9:49 AM

It seems to me that all remarks have already been addressed. Is there anything holding this patch? For it pretty much LGTM.

llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h
50

Why not just 32?

It seems to me that all remarks have already been addressed. Is there anything holding this patch? For it pretty much LGTM.

The major issue is should we accept/merge an extension which is not ratified/standardized.

The major issue is should we accept/merge an extension which is not ratified/standardized.

There are precedents of accepting experimental features in trunk. So the question is rather whether the preliminary RISCV V extensions should be accepted, in trunk or another branch, at this moment or not.

HsiangKai updated this revision to Diff 241956.Sun, Feb 2, 9:37 PM

Update to version 0.8-draft-20191213.

Thanks for updating the patch to the stable v0.8 release (previously version 0.8-draft-20191213). I've added some inline comments.

In addition, we are also missing the whole register move instructions vmv<nf>r.v (for nf = [1,2,4,8]), introduced in version 0.8-draft-20191117.

llvm/lib/Target/RISCV/RISCV.td
62

Since D69899 was commited, we should add the feature name in the AssemblerPredicate parameter and update the missing feature error messages in the tests. Like this:

def HasStdExtV : Predicate<"Subtarget->hasStdExtV()">,                          
                           AssemblerPredicate<"FeatureStdExtV",                 
                           "'V' (Vector Instructions)">;
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
420

As of the stable v0.8 release, the whole register vector loads and stores have been restricted to a single register only. You probably just forgot to change this foreach since vl1r.v and vs1r.v are the only instructions being tested.

After this change you may want to revisit whether the VWholeLoad and VWholeStore multiclasses are still needed.

511

I wonder if this should be VMERGE_V, so that the defined records are VMERGE_VVM, VMERGE_VXM, VMERGE_VIM.

Currently these records are defined VMERGE_VM, VMERGE_XM and VMERGE_IM.

Shouldn't most instructions have the vtype register in Uses? Shouldn't most FP instructions have fflags in Defs?

The tests are extensive, but it would be important to also add negative tests.

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

Please, rename to parseVTypeI().

185

Please, rename to defaultMaskRegOp().

718

This should be the default case for the switch above.

732

Ditto.

814

Avoid calling logBase2() multiple times.

815

Ditto.

1122

Please, rename to Match_InvalidVTypeI.

1127

Please, rename to Match_InvalidVMaskRegister.

1473

Please, rename to parseVTypeI().

1516

Please, rename to parseMaskReg().

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

Please, rename to decodeVMaskReg().

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
164

Please, rename to printVMaskReg().

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
46

Please, rename to printVMaskReg().

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
84

Please, rename to getVMaskReg().

llvm/lib/Target/RISCV/RISCV.td
60

Should it also imply FeatureStdExtD?

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
33

Please, rename to VRegAsmOperand.

40

Please, rename to VRegOp.

45

Please, rename to VMaskAsmOperand.

55

Please, rename to VMaskOp.

78

No need to set fields to the default 0.

535

Shouldn't these have vxsat in Defs?

541

Shouldn't these have vxrm in Uses?

547

Shouldn't this hace vxrm in Uses and vxsat in Defs?

550

Aren't these part of the base V extension?

556

Shouldn't this hace vxrm in Uses?

560

Shouldn't this hace vxrm in Uses and vxsat in Defs?

HsiangKai updated this revision to Diff 242816.EditedThu, Feb 6, 12:17 AM
HsiangKai marked 23 inline comments as done.

Address @fpallares and @evandro's comments.

About fflags in FP instructions, it is also related to RISCVInstrInfoF.td and RISCVInstrInfoD.td. So, I think we could prepare another patch for it.

HsiangKai added inline comments.Thu, Feb 6, 12:18 AM
llvm/lib/Target/RISCV/RISCV.td
60

I didn't find that V will imply D extension. Could you indicate the description in spec about this?

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
78

There is no default value for hasSideEffects, mayLoad, and mayStore.

535

It should have no impact on CodeGen. It seems no instructions to use vxsat.

HsiangKai updated this revision to Diff 242905.Thu, Feb 6, 7:55 AM

Add the predicate for extension Zvqmac.

Thanks for addressing my comments @HsiangKai. After a closer look I've added some more inline comments, and some general ones too:

  • We are missing tests on whole register move instructions vmv<nf>r.v.
  • Overlap diagnosis for those instructions that are missing the register overlap constraints (see inline comments) should be added in RISCVAsmParser::validateInstruction, as well as new tests.
  • I've noticed you are using ${vm} for describing the mask arg on most instructions, but AFAICT $vm is equally valid (e.g. You could write $vd, $vs2$vm instead of $vd, $vs2${vm}).
  • We should probably recognize the assembler pseudo instructions defined in the ISA. Most can be implemented via InstAlias. I suggest to do this in another patch.
  • Instruction from the Zvqmac extension instructions were added, should we add instructions for the other extensions as well? Otherwise we might want to restrict this patch to the base only.
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
154

Following @evandro's naming suggestions, this should probably be renamed to DecodeVRegisterClass.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
40

Whitespace missing before the colon.

55

Ditto.

107

I don't see why setting hasSideEffects is necessary and setting mayStore is not enough, could you please elaborate a bit on this?

426

Whitespace missing before the colon.

438

I'd say those instructions need the @earlyclobber $vd restriction too. Not because of the first source operand (since it has the same width as the destination), but because of the second (in the case of VWOP_WV), or the mask (for VWOP_WX).

Quoting section 11.2. Widening Vector Arithmetic Instructions:

The destination vector register group cannot overlap a source vector register group of a different element width (including the mask register if masked), otherwise an illegal instruction exception is raised.

This has the downside that the earlyclobber constraint is too coarse and will impose unnecessary restrictions by not allowing the destination to overlap with the first (wide) operand, even when the instruction is used unmasked. Maybe we can use the earlyclobber for now, and find a better solution for this in a later patch.

460

In section 11.3. Narrowing Vector Arithmetic Instructions we find the following paragraph:

The destination vector register group cannot overlap the first source vector register group (specified by vs2). The destination vector register group cannot overlap the mask register if used, unless LMUL=1. If either constraint is violated, an illegal instruction exception is raised.

From my understanding, this applies to VNSRL and VNSRA. In that case we need an @earlyclobber constraint here.

480

Whitespace missing before the colon.

557

I'd say overlap restrictions on narrowing instructions apply to VNCLIP and VNCLIPU too.

571

As discussed in another inline comment above (for VWADD_W and such), there is an overlap restriction for these instructions too.

584

Whitespace missing before the colon.

661

I believe we are also missing the @earlyclobber $vd overlap constraints for these narrowing conversions.

From section 14.17. Narrowing Floating-Point/Integer Type-Convert Instructions:

These instructions have the same constraints on vector register overlap as other narrowing instructions (see Narrowing Vector Arithmetic Instructions).

675

If I'm not mistaken, those instructions should have the @earlyclobber $vd constraint too to avoid overlaps.

685

Ditto.

llvm/lib/Target/RISCV/RISCVRegisterInfo.td
50

My understanding is that here you specify minimum sizes (at least we will have one element of size ELEN). As they stand, they are consistent with ELEN=64. So far it is unclear to me what sizes we would want to use if we also want to support ELEN=32. I imagine we could reuse HwMode or something similar.

As far as the offsets are concerned I'd suggest setting them to -1 because we don't really know the offset where the hi part starts.

That said, you already mentioned that types are not that important in this patch.

llvm/test/MC/RISCV/rvv/macc.s
2

Looks like +v attribute gets passed twice. I'm also not sure we need +a and +c.

evandro added inline comments.Tue, Feb 18, 9:22 AM
llvm/lib/Target/RISCV/RISCV.td
60

Indeed it doesn't:

If the base scalar ISA does not include floating-point, then a fcsr register is also added to hold mirrors of the vxsat and vxrm CSRs as explained below.

Which means that the instructions that take FP scalars should also depend on hasStdExt{F|D}. Likewise when the scalar is a 64 bit integer, hasRV64.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
78

Right. It would be cleaner if they were moved to the respective base instruction class then. Just a suggestion.

107

hasSideEffects is not necessary, since mayStore is indeed enough. The purpose of hasSideEffects is as a catch all when the rest of the instruction attributes miss some side effect.

535

For the sake of completeness.

rogfer01 added inline comments.Tue, Feb 18, 1:00 PM
llvm/lib/Target/RISCV/RISCV.td
60

I don't think we have to do anything special with 64 bit: section 11.1 states what is the behaviour when XLEN is different than SEW.

evandro added inline comments.Tue, Feb 18, 1:24 PM
llvm/lib/Target/RISCV/RISCV.td
60

Good point. ๐Ÿ‘๐Ÿป

HsiangKai marked 3 inline comments as done.Wed, Feb 19, 12:09 AM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
154

The function name will be expected by TableGen as "Decode" + Record->getName().str() + "RegisterClass". It seems related to the name of register class. Currently, the name of RegisterClass for V-extension registers is "VR".

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
107

There is no default value for hasSideEffects, mayLoad, and mayStore in Instruction class. So, I specify these three values for V instructions. It follows the style in RISCVInstrInfo.td. I agree that we could give these attributes some default values in some base class for V instructions. I will try to refactor it.

460

In current implementation for MC layer, there is no LMUL information in these instructions. So, if LMUL=1, there is no such restriction for them. I have a downstream patch to extend these instruction definitions for LMUL > 1. Maybe we could keep this MC implementation for LMUL=1 and extend it later. I will add comments for it.

Address some of @fpallares' comments.

Remove Zvqmac extension. Keep this patch for base only.

rogfer01 added inline comments.Wed, Feb 19, 12:47 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
460

Perhaps the wording in the spec is unclear here?

Looks like the following sentence does apply regardless of LMUL.

The destination vector register group cannot overlap the first source vector register group (specified by vs2).

While the sentence that follows it is clear that applies only to LMUL>1

The destination vector register group cannot overlap the mask register if used, unless LMUL=1

You also said that

I have a downstream patch to extend these instruction definitions for LMUL > 1

I'm curious here: did you use a Pseudo for that or somehow avoided the issue that they'd be encoded in the same way?

Thanks!

fpallares marked an inline comment as done.Wed, Feb 19, 2:23 AM
fpallares added inline comments.
llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
154

I see, sorry for the noise.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
107

I was only referring tot he fact that hasSideEffects here changed from 0 to 1 in a recent update of the patch. I don't really think a refactor is necessary here.

HsiangKai marked 2 inline comments as done.Wed, Feb 19, 7:33 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCV.td
60

Yes, it is a problem about accessing fcsr if F extension is turned off. However, I found that there is vcsr in the latest version.

https://github.com/riscv/riscv-v-spec/commit/b25b643b9c29b4fde570293c64ca682a99a09f2a

So, I think we could keep V and F as separate unrelated extensions.

I will add predicates for vector floating point operations.

llvm/lib/Target/RISCV/RISCVInstrInfoV.td
460

I misunderstood the statements. Sorry for that. I will add earlyclobber for narrowing instructions. Thanks a lot.

About instruction definitions for LMUL > 1, I enable 'isCodeGenOnly' for these instructions to avoid encoding checking.

HsiangKai updated this revision to Diff 245568.Wed, Feb 19, 7:38 PM
  • Add predicates for vector floating-point instructions.
  • Add earlyclobber for narrowing instructions.
HsiangKai updated this revision to Diff 245571.Wed, Feb 19, 8:40 PM

Update test cases.