This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Assemble/Disassemble v-ext instructions.
ClosedPublic

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

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Isn't that already done by virtue of the uses of llvm-objdump -d - in the existing tests?

Oh yes. Sorry for the noise.

HsiangKai marked 2 inline comments as done.Nov 18 2019, 12:05 AM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1664

It makes sense. I will modify the patch according to your comments. Thanks a lot.

2276

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.Feb 2 2020, 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
154

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
159

Please, rename to parseVTypeI().

209

Please, rename to defaultMaskRegOp().

766

This should be the default case for the switch above.

780

Ditto.

860

Avoid calling logBase2() multiple times.

861

Ditto.

1181

Please, rename to Match_InvalidVTypeI.

1186

Please, rename to Match_InvalidVMaskRegister.

1547

Please, rename to parseVTypeI().

1590

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
45

Please, rename to printVMaskReg().

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

Please, rename to getVMaskReg().

llvm/lib/Target/RISCV/RISCV.td
152

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.EditedFeb 6 2020, 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.Feb 6 2020, 12:18 AM
llvm/lib/Target/RISCV/RISCV.td
152

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.Feb 6 2020, 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.Feb 18 2020, 9:22 AM
llvm/lib/Target/RISCV/RISCV.td
152

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.Feb 18 2020, 1:00 PM
llvm/lib/Target/RISCV/RISCV.td
152

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.Feb 18 2020, 1:24 PM
llvm/lib/Target/RISCV/RISCV.td
152

Good point. 👍🏻

HsiangKai marked 3 inline comments as done.Feb 19 2020, 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.Feb 19 2020, 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.Feb 19 2020, 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.Feb 19 2020, 7:33 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCV.td
152

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.Feb 19 2020, 7:38 PM
  • Add predicates for vector floating-point instructions.
  • Add earlyclobber for narrowing instructions.
HsiangKai updated this revision to Diff 245571.Feb 19 2020, 8:40 PM

Update test cases.

fpallares added inline comments.Mar 6 2020, 2:20 AM
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 marked an inline comment as done.Mar 10 2020, 8:43 AM
HsiangKai added inline comments.
llvm/test/MC/RISCV/rvv/invalid.s
6

Got it. Thanks.

HsiangKai updated this revision to Diff 249399.Mar 10 2020, 8:45 AM

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

  • Rewrite validateInstruction() in RISCVAsmParser.cpp. Use TSFlags to classify instructions' constraints.
  • Update invalid.s test cases.
HsiangKai updated this revision to Diff 254501.Apr 2 2020, 6:27 AM
  • Rebase on master branch.
  • Fix validInstruction() bugs.
  • Update test cases.
HsiangKai updated this revision to Diff 254667.Apr 2 2020, 6:23 PM
  • Fix clang-format errors.
  • Rename variables according to LLVM Coding Standards.
HsiangKai updated this revision to Diff 258526.Apr 18 2020, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2020, 8:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
simoncook added a reviewer: simoncook.EditedApr 21 2020, 2:26 AM

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 updated this revision to Diff 259494.Apr 23 2020, 1:09 AM
  • 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 updated this revision to Diff 259496.Apr 23 2020, 1:46 AM
  • Refine naming.
  • Some typo.
HsiangKai updated this revision to Diff 262688.May 7 2020, 10:15 AM

Rebase on master.

Fix clang-tidy warnings.

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

@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)?

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.

This comment was removed by 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 marked an inline comment as done.May 20 2020, 2:29 PM
HsiangKai added inline comments.
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?

HsiangKai edited the summary of this revision. (Show Details)May 20 2020, 3:24 PM
fpallares added inline comments.May 21 2020, 12:52 AM
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.

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 updated this revision to Diff 266771.May 28 2020, 1:38 AM
  • Check operands for widening and narrowing instructions.
  • Modify according to @fpallares's comments.
HsiangKai updated this revision to Diff 266826.May 28 2020, 5:32 AM

Apply clang-format.

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 updated this revision to Diff 268557.Jun 4 2020, 12:22 PM

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.

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 added inline comments.Jun 4 2020, 3:29 PM
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 marked an inline comment as done.Jun 4 2020, 7:07 PM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCVInstrFormats.td
56

Do you mean

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

HsiangKai updated this revision to Diff 268641.Jun 4 2020, 7:40 PM

Remove redundant RVVConstraint.

HsiangKai updated this revision to Diff 268651.Jun 4 2020, 8:18 PM

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

Any further comments @simoncook @asb?

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

evandro added inline comments.Jun 15 2020, 12:39 PM
llvm/lib/Target/RISCV/RISCVInstrFormats.td
56

Yes.

asb accepted this revision.Jun 25 2020, 5:52 AM

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.

This revision is now accepted and ready to land.Jun 25 2020, 5:52 AM
asb added inline comments.Jun 25 2020, 5:56 AM
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 marked an inline comment as done.Jun 25 2020, 7:34 AM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCVInstrFormats.td
56

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

HsiangKai marked an inline comment as done.Jun 25 2020, 7:36 AM
HsiangKai added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
10

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

evandro added inline comments.Jun 25 2020, 8:25 AM
llvm/lib/Target/RISCV/RISCVInstrFormats.td
56

Please, address this comment before committing.

This revision was automatically updated to reflect the committed changes.