[RISCV] Fix RISCVInstrInfo::getInstSizeInBytes for atomics pseudos

[jrtc27]

Summary

Without these, the generic branch relaxation pass will underestimate the
range required for branches spanning these and we can end up with
"fixup value out of range" errors rather than relaxing the branches.
Some of the instructions in the expansion may end up being compressed
but exactly determining that is awkward, and these conservative values
should be safe, if slightly suboptimal in rare cases.

[jrtc27]

Correct comment.

[MaskRay]

LGTM. I guess some may be difficult to test. CodeGen/RISCV/branch-relaxation.ll CodeGen/AArch64/branch-relax-bcc.ll ...

[luismarques]

Thanks for the fix Jessica.
Looking at, say, doAtomicBinOpExpansion it's not immediately obvious why some of those values are correct. Can you please provide more info, ideally in the code?
Is there really no way to provide tests for these?

[lewis-revill]

Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.

[jrtc27]

In D77443#2118980, @lewis-revill wrote:

Bumping this patch - Looking at the output of the riscv-expand-pseudos pass I'd say these values are accurate. I'd say comments here, and in the functions which expand the instructions to ensure things don't get changed - seems to me like tests would be a little tricky so perhaps omitting that would be fine just to get this landed.

Ugh yes I never followed up on this properly. I started trying to write test but it seemed really fragile; the tests could maybe enforce they were _approximately_ correct but at the IR level there's just not enough control to know exactly how many bytes you're trying to create a branch for. Maybe MIR tests would work. If someone wants to pick this up and add tests by all means please do, but I don't have the time available to do that myself unfortunately.

[luismarques]

Alright, let's get this landed! Please just add comments in the expansion functions, noting the need to update these values if the expansion templates ever change, as Lewis says.

[asb]

LGTM, +1 on adding a comment to the expansion functions noting the need to update getInstSizeInBytes. Thanks!

[asb]

Hi @jrtc27 - it would be nice to get this in before LLVM 11 branches. Will you have time to add the extra comment and commit today?

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

bool RISCVExpandAtomicPseudo::expandMBB(MachineBasicBlock &MBB) {
   }
 
   return Modified;
 }
 
 bool RISCVExpandAtomicPseudo::expandMI(MachineBasicBlock &MBB,
                                        MachineBasicBlock::iterator MBBI,
                                        MachineBasicBlock::iterator &NextMBBI) {
+  // RISCVInstrInfo::getInstSizeInBytes hard-codes the number of expanded
+  // instructions for each pseudo, and must be updated when adding new pseudos
+  // or changing existing ones.
   switch (MBBI->getOpcode()) {
   case RISCV::PseudoAtomicLoadNand32:
     return expandAtomicBinOp(MBB, MBBI, AtomicRMWInst::Nand, false, 32,
                              NextMBBI);
   case RISCV::PseudoAtomicLoadNand64:
     return expandAtomicBinOp(MBB, MBBI, AtomicRMWInst::Nand, false, 64,
                              NextMBBI);
   case RISCV::PseudoMaskedAtomicSwap32:

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

bool RISCVExpandPseudo::expandMBB(MachineBasicBlock &MBB) {
   }
 
   return Modified;
 }
 
 bool RISCVExpandPseudo::expandMI(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator MBBI,
                                  MachineBasicBlock::iterator &NextMBBI) {
+  // RISCVInstrInfo::getInstSizeInBytes hard-codes the number of expanded
+  // instructions for each pseudo, and must be updated when adding new pseudos
+  // or changing existing ones.
   switch (MBBI->getOpcode()) {
   case RISCV::PseudoLLA:
     return expandLoadLocalAddress(MBB, MBBI, NextMBBI);
   case RISCV::PseudoLA:
     return expandLoadAddress(MBB, MBBI, NextMBBI);
   case RISCV::PseudoLA_TLS_IE:
     return expandLoadTLSIEAddress(MBB, MBBI, NextMBBI);
   case RISCV::PseudoLA_TLS_GD:

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

default: {
     }
     return get(Opcode).getSize();
   }
   case TargetOpcode::EH_LABEL:
   case TargetOpcode::IMPLICIT_DEF:
   case TargetOpcode::KILL:
   case TargetOpcode::DBG_VALUE:
     return 0;
+  // These values are determined based on RISCVExpandAtomicPseudoInsts,
+  // RISCVExpandPseudoInsts and RISCVMCCodeEmitter, depending on where the
+  // pseudos are expanded.
   case RISCV::PseudoCALLReg:
   case RISCV::PseudoCALL:
   case RISCV::PseudoJump:
   case RISCV::PseudoTAIL:
   case RISCV::PseudoLLA:
   case RISCV::PseudoLA:
   case RISCV::PseudoLA_TLS_IE:
   case RISCV::PseudoLA_TLS_GD:
     return 8;
+  case RISCV::PseudoAtomicLoadNand32:
+  case RISCV::PseudoAtomicLoadNand64:
+    return 20;
+  case RISCV::PseudoMaskedAtomicSwap32:
+  case RISCV::PseudoMaskedAtomicLoadAdd32:
+  case RISCV::PseudoMaskedAtomicLoadSub32:
+    return 28;
+  case RISCV::PseudoMaskedAtomicLoadNand32:
+    return 32;
+  case RISCV::PseudoMaskedAtomicLoadMax32:
+  case RISCV::PseudoMaskedAtomicLoadMin32:
+    return 44;
+  case RISCV::PseudoMaskedAtomicLoadUMax32:
+  case RISCV::PseudoMaskedAtomicLoadUMin32:
+    return 36;
+  case RISCV::PseudoCmpXchg32:
+  case RISCV::PseudoCmpXchg64:
+    return 16;
+  case RISCV::PseudoMaskedCmpXchg32:
+    return 32;
   case TargetOpcode::INLINEASM:
   case TargetOpcode::INLINEASM_BR: {
     const MachineFunction &MF = *MI.getParent()->getParent();
     const auto &TM = static_cast<const RISCVTargetMachine &>(MF.getTarget());
     return getInlineAsmLength(MI.getOperand(0).getSymbolName(),
                               *TM.getMCAsmInfo());
   }
   }