Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[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

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.