[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Evandro Menezes via Phabricator via cfe-commits]

evandro added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:56
+def NoConstraint : RISCVVConstraint<0>;
+def WidenV   : RISCVVConstraint<1>;
+def WidenW   : RISCVVConstraint<2>;

HsiangKai wrote:
> evandro wrote:
> > HsiangKai wrote:
> > > evandro wrote:
> > > > 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.
> > > Do you mean
> > > 
> > > WidenV = Widen;
> > > WidenW = Widen | WideInput;
> > > WidenCvt = Widen | Cvt;
> > Yes.
> Got it. I will improve it based on v0.9 implementation.
Please, address this comment before committing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai marked an inline comment as done.
HsiangKai added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:9
+///
+/// This file describes the RISC-V instructions from the standard 'V',
+/// Vector instruction set extension.

asb wrote:
> 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.
Got it. I will add more comments similar to RISCVInstrInfoB.td before landing 
the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai marked an inline comment as done.
HsiangKai added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:56
+def NoConstraint : RISCVVConstraint<0>;
+def WidenV   : RISCVVConstraint<1>;
+def WidenW   : RISCVVConstraint<2>;

evandro wrote:
> HsiangKai wrote:
> > evandro wrote:
> > > 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.
> > Do you mean
> > 
> > WidenV = Widen;
> > WidenW = Widen | WideInput;
> > WidenCvt = Widen | Cvt;
> Yes.
Got it. I will improve it based on v0.9 implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Alex Bradbury via Phabricator via cfe-commits]

asb added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:9
+///
+/// This file describes the RISC-V instructions from the standard 'V',
+/// Vector instruction set extension.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Alex Bradbury via Phabricator via cfe-commits]

asb accepted this revision.
asb added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Evandro Menezes via Phabricator via cfe-commits]

evandro added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:56
+def NoConstraint : RISCVVConstraint<0>;
+def WidenV   : RISCVVConstraint<1>;
+def WidenW   : RISCVVConstraint<2>;

HsiangKai wrote:
> evandro wrote:
> > 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.
> Do you mean
> 
> WidenV = Widen;
> WidenW = Widen | WideInput;
> WidenCvt = Widen | Cvt;
Yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Ferran Pallarès Roca via Phabricator via cfe-commits]

fpallares added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

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

Any further comments @simoncook @asb?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai marked an inline comment as done.
HsiangKai added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:56
+def NoConstraint : RISCVVConstraint<0>;
+def WidenV   : RISCVVConstraint<1>;
+def WidenW   : RISCVVConstraint<2>;

evandro wrote:
> 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.
Do you mean

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Evandro Menezes via Phabricator via cfe-commits]

evandro added inline comments.



Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:56
+def NoConstraint : RISCVVConstraint<0>;
+def WidenV   : RISCVVConstraint<1>;
+def WidenW   : RISCVVConstraint<2>;

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.



Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:61
+def Narrow   : RISCVVConstraint<5>;
+def NarrowCvt: RISCVVConstraint<6>;
+def Iota : RISCVVConstraint<7>;

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Evandro Menezes via Phabricator via cfe-commits]

evandro added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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




Comment at: llvm/test/MC/RISCV/rvv/add.s:1
+# RUN: llvm-mc -triple=riscv64 -show-encoding --mattr=+experimental-v < %s \
+# RUN:| FileCheck %s --check-prefixes=CHECK-ENCODING,CHECK-INST

`< %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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Ferran Pallarès Roca via Phabricator via cfe-commits]

fpallares added a comment.

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.




Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1078
   case Match_InvalidUImm5:
 return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 5) - 1);
   case Match_InvalidSImm6:

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]`)



Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:296
+
+def VDataVT : RegisterTypes<[nxv8i8, nxv16i8, nxv32i8,
+nxv4i16, nxv8i16, nxv16i16, nxv32i16,

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



Comment at: llvm/test/MC/RISCV/rvv/add.s:324
+vwcvt.x.x.v v8, v4
+# CHECK-INST: vwadd.vx v8, v4
+# CHECK-ENCODING: [0x57,0x64,0x40,0xc6]

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Ferran Pallarès Roca via Phabricator via cfe-commits]

fpallares added inline comments.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2283
+  unsigned Src2Reg = Inst.getOperand(1).getReg();
+  if (DestReg == Src2Reg)
+return Error(Loc, "The destination vector register group cannot 
overlap"

HsiangKai wrote:
> fpallares wrote:
> > 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)
> > 
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai marked an inline comment as done.
HsiangKai added inline comments.



Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2283
+  unsigned Src2Reg = Inst.getOperand(1).getReg();
+  if (DestReg == Src2Reg)
+return Error(Loc, "The destination vector register group cannot 
overlap"

fpallares wrote:
> 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)
> 
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Ferran Pallarès Roca via Phabricator via cfe-commits]

fpallares added a comment.

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




Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2283
+  unsigned Src2Reg = Inst.getOperand(1).getReg();
+  if (DestReg == Src2Reg)
+return Error(Loc, "The destination vector register group cannot 
overlap"

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)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Ferran Pallarès Roca via Phabricator via cfe-commits]

fpallares added a comment.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69987: [RISCV] Assemble/Disassemble v-ext instructions.

[Simon Cook via Phabricator via cfe-commits]

simoncook added a reviewer: simoncook.
simoncook added a comment.

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.

One question, I appreciate the Zvediv/Zvqmac sections say they are not planned 
to be part of the base V extension, but is it worth having them too behind 
separate flags so they can be used/evaluated. (I'm thinking like the 
Zbproposedc extension we added for bitmanip, which is not part of the proposed 
finalized 'B', but is included for comparisons).

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 `InstAlias`es to be defined for these and land in the same patch.




Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:160
+
+  Register Reg = RISCV::V0 + RegNo;
+  Inst.addOperand(MCOperand::createReg(Reg));

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.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:449
+}
+}
+

Add comment noting end of earlyclobber/constraint scope



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:532
+// Vector Integer Move Instructions
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+// op vd, vs1

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



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:551
+}
+}
+

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



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:578
+
+// Vector Single-Width Integer Reduction Instructions
+defm VREDSUM : VALU_MV_V<"vredsum", 0b00>;

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.



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:648
+
+}
+

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



Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoV.td:651
+foreach nf = [1, 2, 4, 8] in {
+  def VMV#nf#R_V  : RVInstV<0b100111, !add(nf, -1), OPIVI, (outs VRegOp:$vd),
+(ins VRegOp:$vs2), "vmv" # nf # "r.v",

This should be below vcompress?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69987/new/

https://reviews.llvm.org/D69987



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits