[PATCH] D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In https://reviews.llvm.org/D42780#1050057, @sabuasal wrote:

> In https://reviews.llvm.org/D42780#1039073, @asb wrote:
>
> > Looking at RISCVGenCompressInstEmitter:
> >
> > - I'm not sure I see the need for the RISCVAreEqualOperands helper. You already don't consistently check that every operand isReg. I think it's fine to rely on invalid operands at this stage being a programming error which would be picked up in an asserts build by getReg presumably.
>
>
> This function is added to check for tied operands, weather it being a register or immediate. for example , if you use the patters (INSTR $rs  $rs ) we need a check that these two operands are equal in value (if resisters, hold the same register, if immediates hold the same immediate) .  Currently RISCV doesn't have timed immediate operands but this backend is written keeping in mind that new patterns or different ISAs might use it.
>
> so in case Immediate check is need this function should be updated to: 
>  return ( MCOp1.isReg() && MCOp2.isReg() &&
>
>      (MCOp1.getReg() == MCOp2.getReg())  ) || 
>   ( MCOp1.isImm() && MCOp2.isImm() &&
>      (MCOp1.getImm() == MCOp2.getImm())
>   
>
> What places do you think we should have checked for an operand being a register? the combination of validation time (compile the td file) and runtime check should catch all cases I think. Maybe we are missing something?


Most of the logic other than RISCVAreEqualOperands seems to just use .getReg with no guard. e.g. the generated if statement for add -> c.mv:

  if (STI.getFeatureBits()[RISCV::FeatureStdExtC] &&
    (MI.getOperand(1).getReg() == RISCV::X0) &&
    (MRI.getRegClass(RISCV::GPRNoX0RegClassID).contains(MI.getOperand(0).getReg())) &&
    (MRI.getRegClass(RISCV::GPRNoX0RegClassID).contains(MI.getOperand(2).getReg()))) {

Really I was suggesting that the helper seems unnecessary, and you might as well just directly emit MCOp1.getReg() == MCOp2.getReg() into the if statement.


https://reviews.llvm.org/D42780