This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] MC layer support for the rest instructions of standard compress instruction set
ClosedPublic

Authored by shiva0217 on Nov 13 2017, 11:26 PM.

Details

Summary
  1. Add CIW encoding format for C extension instructions
  2. Add IsRV32 predicate for RV32 only instructions
  3. Add the rest C extension instructions
  4. Identify/check UImm5NonZero/UImm6NonZero/SImm6/SImm10Lsb0000/UImm10Lsb00NonZero operands by RISCVAsmParser
  5. Add imply SP operands when disassembling CADDI4SPN, CADDI16SP, CFLWSP, CFSWSP CFLDSP, CFSDSP instructions
  6. Add GPRNoX0X2, FPR32C, FPR64C register classes and support decode these register classes
  7. Use DecoderTableRISCV32Only_16 to decode compress RV32 only instructions to avoid decode conflict because some RV32 and RV64 compress instructions using same encoding to compact the encoding space
  8. Add MC testcases for the rest C extension instructions

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Nov 13 2017, 11:26 PM
shiva0217 updated this revision to Diff 123117.Nov 15 2017, 5:36 PM
apazos added a subscriber: apazos.Nov 17 2017, 1:08 PM
apazos added inline comments.
test/MC/RISCV/fixups-compressed.s
1

In previous patches you corrected the tests to set -triple and -mattr

Hi Shiva,
I think you can add llvm-commits as subscriber to your patches.
Before, you might want to run clang-format to catch indentation issues.
And make sure you push patches with all the context (git diff HEAD~1 HEAD -U9999999999).
Thank you!

shiva0217 added a subscriber: llvm-commits.

Hi Ana,
I add llvm-commits as subscriber and use clang-format to catch indentation issues.
And also update the patches with all the context (git diff HEAD~1 HEAD -U9999999999).
Thanks for your comments I really appreciate that.

test/MC/RISCV/fixups-compressed.s
1

Add missing -triple argument in the test case.

apazos added inline comments.Nov 20 2017, 2:17 PM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
281

shouldn't this one go after isSImm13Lsb0()?

shiva0217 updated this revision to Diff 123699.Nov 20 2017, 5:34 PM
shiva0217 added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
281

Move isSImm10Lsb0000 after isSImm13Lsb0

shiva0217 edited the summary of this revision. (Show Details)
  1. Update test cases that test the immediate value max/min in -valids.s and just outside the range in -invalid.s.
  1. List the instruction patterns base on the table in section 12.7 "RVC Instruction Set Listings" (page 82 and 83).
  1. Update immediate operand constraints UImm5NonZero/UImm6NonZero/UImm10Lsb00NonZero to reflect that the instructions do not allow zero immediate value.
shiva0217 edited the summary of this revision. (Show Details)
  1. Add test case to check the instruction operands should not x0.
  1. Add GPRNonX0X2 register class and test case for c.lui.
asb edited edge metadata.Dec 7 2017, 5:46 AM

Hi Shiva, I think this is basically ready to merge. I notice that it's missing the compressed floating point instructions - now the the F+D extensions are added, did you want to add those?

Check the indentation in RISCVDisassembler.cpp, and please check UImm10Lsb00NonZero and SImm6 are sorted properly with respect to the other immediate types. I'd rename GPRNonX0X2 to GPRNoX0X2.

shiva0217 updated this revision to Diff 126491.Dec 11 2017, 6:22 PM
shiva0217 edited the summary of this revision. (Show Details)

Hi Alex, I add the floating compress instructions after F+D floating patches landing and add IsRV32 predicate for RV32 only compress instructions.
Some RV32 only instruction using the same encoding with RV64 instructions to compact the compress instruction coding space. To avoid decode conflict, I split these instructions to another decode table.

shiva0217 updated this revision to Diff 126495.Dec 11 2017, 6:38 PM

Fix indentation in RISCVDisassembler.cpp

asb added inline comments.Dec 12 2017, 6:57 AM
lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
164

Rather than introducing the isRV32only function which does its own minimal parsing and duplicates some information in Tablegen, can't we just use DecoderTableRISCV32Only_16 whenever we are targeting RV32, and then have Decoder16 as a fallback? Something like:

if (STI.getFeatureBits()[RISCV::Feature32Bit]) {
  DEBUG(dbgs() << "Trying RISCV32Only_16 table (16-bit Instruction):\n");
  // Calling the auto-generated decoder function.
  Result = decodeInstruction(DecoderTableRISCV32Only_16, MI, Insn, Address,
                             this, STI);
  if (Result != MCDisassembler::Fail) {
    Size = 2;
    return Result;
  }
}

DEBUG(dbgs() << "Trying RISCV_C table (16-bit Instruction):\n");
// Calling the auto-generated decoder function.
Result = decodeInstruction(DecoderTable16, MI, Insn, Address, this, STI);
lib/Target/RISCV/RISCV.td
49 ↗(On Diff #126495)

We might want to change the subtargetfeatures at some point, and introduce a new enum but I'd prefer to leave it with the simple boolean value in this patch. I can see how an enum might have some advantages, but I don't think the change belongs in this patch. Plus our HwModes are currently defined in terms of the presence or absence of Feature64Bit.

In the future, it may make sense to have a feature bit which indicates the base ISA (RV32I, RV32E, RV64I), particularly as it's possible there will be RV32EC-only encodings.

shiva0217 updated this revision to Diff 126663.Dec 12 2017, 5:45 PM
shiva0217 added inline comments.Dec 12 2017, 5:50 PM
lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
164

Yes, we could implement this way to avoid introducing parsing by isRV32only function.
Thanks.

lib/Target/RISCV/RISCV.td
49 ↗(On Diff #126495)

Ok, we could leave the change enum until more Arch configures involved.

asb accepted this revision.Dec 13 2017, 1:25 AM

Thanks Shiva! Looks good to me.

This revision is now accepted and ready to land.Dec 13 2017, 1:25 AM
This revision was automatically updated to reflect the committed changes.