jrtc27 added inline comments.
================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:442
+
+ if (STI.getFeatureBits()[RISCV::FeatureExtZpsfoperand] &&
+ !STI.getFeatureBits()[RISCV::Feature64Bit]) {
----------------
Jim wrote:
> jrtc27 wrote:
> > The table is called RISCV32POnly but you're checking for Zpsfoperand
> > (whatever that mouthful of an extension is). Which is it?
> Rename RISCV32POnly to RISCV32Zpsfoperand.
> This is for the instruction with even/odd paired-register operands on RV32.
>
> In RISCV32Zpsfoperand, two kinds of instruction defined for the same
> instruction in spec, one for RV32 with even/odd paired-register operands
> defined in RISCV32Zpsfoperand table , the other one for RV64 with normal GPR
> operands.
>
>
Is there a reason why this needs to be limited to Zpsfoperand? We don't have
separate namespaces for every extension, we only add them when there are
conflicts (with the "more common" extension being in the default namespace).
I'd expect RISCV32Only_ with just a Feature64Bit check, like we do for some
RV32C instructions, to be sufficient.
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:10
+/// This file describes the RISC-V instructions from the standard 'P' SIMD
+/// extension, version 0.96.
+/// This version is still experimental as the 'P' extension hasn't been
----------------
Isn't it 0.9.6 (or 0.9.7 now)? Not that that's a legal extension version
number...
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:147
+ Predicates = [HasStdExtZpsfoperand, IsRV32] in
+ def "32" : RVPALU64Pair<funct7, funct3, opcodestr>;
+ let Predicates = [HasStdExtZpsfoperand, IsRV64] in
----------------
FOO32 vs FOO64 intuitively implies something about the width of operands to me,
not what ISA they're for. For B the instruction definitions use _RV32 and _RV64
suffices.
Especially since the instructions are already called things like ADD64, this
will generate nonsense ADD6432 and ADD6464 names...
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:169
+ : RVInstR<funct7, funct3, OPC_OP_P, (outs GPR32PairOp:$rd),
+ (ins GPR:$rs1, GPR:$rs2),
+ opcodestr, "$rd, $rs1, $rs2">;
----------------
This, RVPSMAL64Pair and RVPALU64Pair are all just RVPBinary with different
numbers of the operands changed to GPR32PairOp, I think this would be better
with default (to GPR) template arguments added to RVPBinary for rd/rs1/rs2
rather than having three almost-identical copies of the same thing under
varying names
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:181
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVPMA64Pair<bits<7> funct7, bits<3> funct3, string opcodestr>
+ : RVInstR<funct7, funct3, OPC_OP_P, (outs GPR32PairOp:$rd),
----------------
Similarly just make RVPTernary take template arguments for the types (in this
case only one argument, for rd/rs3, needed)
================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoP.td:909
+
+// kmar64 has a aliased instruction kmada32 belong to zpn sub-extension on
RV64.
+let DecoderNamespace = "RISCV32Zpsfoperand_",
----------------
Having the same instruction in two different extensions under two different
names is insane. Currently this implementation lets you use the "wrong" name
for kmar64 with Zpn. But I would prefer the spec weren't crazy.
================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:219
+def GPR32Pairs : RegisterTuples<[gpr32_pair_lo, gpr32_pair_hi],
+ [(add X0, X2, X4, X6,
+ X8, X10, X12, X14,
----------------
Won't the order affect register allocation? Technically not an issue for this
MC patch but the correct thing should be committed so there's no churn when
adding codegen.
That or don't reuse this in the register class below and instead manually list
the pairs in the class.
================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:228
+
+def GPR32Pair : RegisterClass<"RISCV", [untyped], 64, (add GPR32Pairs)> {
+ let Size = 64;
----------------
Why is this untyped?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D95588/new/
https://reviews.llvm.org/D95588
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
