[PATCH] D40003: [RISCV] MC layer support for the rest instructions of standard compress instruction set

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 added inline comments.


================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:281
+
+    if (isRV32Only(Insn, STI)) {
+      DEBUG(dbgs() << "Trying RISCV32Only_16 table (16-bit Instruction):\n");
----------------
asb wrote:
> 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);
> ```
Yes, we could implement this way to avoid introducing parsing by isRV32only function.
Thanks.


================
Comment at: lib/Target/RISCV/RISCV.td:49
 def Feature64Bit
-    : SubtargetFeature<"64bit", "HasRV64", "true", "Implements RV64">;
+    : SubtargetFeature<"64bit", "RISCVArchVersion", "RV64", "Implements RV64">;
 def IsRV64 : Predicate<"Subtarget->is64Bit()">,
----------------
asb wrote:
> 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.
Ok, we could leave the change enum until more Arch configures involved.


Repository:
  rL LLVM

https://reviews.llvm.org/D40003