[RISC-V] Implement RISCVInstrInfo::isCopyInstrImpl()

[arichardson]

Summary

This does not result in changes for any of the current tests, but it might
improve debug information in some cases.

[luismarques]

This is technically correct, but looking at the other targets it seems that the hook is generally being used more restrictively for instructions where you actually have some expectation of it being a copy/move. Do we really want to go down this route? In principle we'd have to later expand this to e.g. all of the bitmanip instructions that can implement copies and so on, which seems a bit like a reductio ad absurdum. Having tests showing at least a little bit of benefit would help make the case for this patch :-)

[arichardson]

Drop additional cases, keeping only the ones also listed in isAsCheapAsAMove().

[luismarques]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

541–542

Do the ORI, XORI cases also provide a measurable benefit here, as they do for isAsCheapAsAMove? Taking a step back, is there a principled way to decide which instructions we include? I can see two cases that are easy to defend: (1) keeping only canonical move instructions; (2) adding all instructions that provide some benefit. Without tests it's not obvious to me that ORI and XORI fit the second case. Did you check if the commits for other uses of this hook had tests that we could adapt?

[luismarques]

I was hoping others would chime in regarding what instructions should be handled in this hook.

@arichardson How do you feel about removing the ORI/XORI and landing this patch? (I also have no major objections to just landing as is). We can always further improve it later. Ideally this would add tests, but if that's not easy I think it's OK to land it as is.

[arichardson]

In D86522#2278710, @luismarques wrote:

I was hoping others would chime in regarding what instructions should be handled in this hook.

@arichardson How do you feel about removing the ORI/XORI and landing this patch? (I also have no major objections to just landing as is). We can always further improve it later. Ideally this would add tests, but if that's not easy I think it's OK to land it as is.

Sure, will do that.

[jrtc27]

In D86522#2278718, @arichardson wrote:

In D86522#2278710, @luismarques wrote:

I was hoping others would chime in regarding what instructions should be handled in this hook.

@arichardson How do you feel about removing the ORI/XORI and landing this patch? (I also have no major objections to just landing as is). We can always further improve it later. Ideally this would add tests, but if that's not easy I think it's OK to land it as is.

Sure, will do that.

Yeah I agree without further info I think we should land just the canonical moves.

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

548

llvm/lib/Target/RISCV/RISCVInstrInfo.h

public:
 
   MachineBasicBlock *getBranchDestBlock(const MachineInstr &MI) const override;
 
   bool isBranchOffsetInRange(unsigned BranchOpc,
                              int64_t BrOffset) const override;
 
   bool isAsCheapAsAMove(const MachineInstr &MI) const override;
 
+  Optional<DestSourcePair>
+  isCopyInstrImpl(const MachineInstr &MI) const override;
+
   bool verifyInstruction(const MachineInstr &MI,
                          StringRef &ErrInfo) const override;
 
   bool getMemOperandWithOffsetWidth(const MachineInstr &LdSt,
                                     const MachineOperand *&BaseOp,
                                     int64_t &Offset, unsigned &Width,
                                     const TargetRegisterInfo *TRI) const;
 

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

 
 bool RISCVInstrInfo::isAsCheapAsAMove(const MachineInstr &MI) const {
   const unsigned Opcode = MI.getOpcode();
   switch (Opcode) {
   default:
     break;
   case RISCV::FSGNJ_D:
   case RISCV::FSGNJ_S:
-    // The canonical floatig-point move is fsgnj rd, rs, rs.
+    // The canonical floating-point move is fsgnj rd, rs, rs.
     return MI.getOperand(1).isReg() && MI.getOperand(2).isReg() &&
            MI.getOperand(1).getReg() == MI.getOperand(2).getReg();
   case RISCV::ADDI:
   case RISCV::ORI:
   case RISCV::XORI:
     return (MI.getOperand(1).isReg() &&
             MI.getOperand(1).getReg() == RISCV::X0) ||
            (MI.getOperand(2).isImm() && MI.getOperand(2).getImm() == 0);
   }
   return MI.isAsCheapAsAMove();
 }
 
+Optional<DestSourcePair>
+RISCVInstrInfo::isCopyInstrImpl(const MachineInstr &MI) const {
+  if (MI.isMoveReg())
+    return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+  switch (MI.getOpcode()) {
+  default:
+    break;
+  case RISCV::ADDI:
+    if (MI.getOperand(2).isImm() && MI.getOperand(2).getImm() == 0)
+      return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+    break;
+  case RISCV::FSGNJ_D:
+  case RISCV::FSGNJ_S:
+    // The canonical floating-point move is fsgnj rd, rs, rs.
+    if (MI.getOperand(1).isReg() && MI.getOperand(2).isReg() &&
+        MI.getOperand(1).getReg() == MI.getOperand(2).getReg())
+      return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
+    break;
+  }
+  return None;
+}
+
 bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
                                        StringRef &ErrInfo) const {
   const MCInstrInfo *MCII = STI.getInstrInfo();
   MCInstrDesc const &Desc = MCII->get(MI.getOpcode());
 
   for (auto &OI : enumerate(Desc.operands())) {
     unsigned OpType = OI.value().OperandType;
     if (OpType >= RISCVOp::OPERAND_FIRST_RISCV_IMM &&