getInstSizeInBytes: sentinel value fix for AArch64, AMDGPU, MSP430

[SjoerdMeijer]

Summary

In commit rL277126, target hook getInstSizeInBytes was added to TargetInstrInfo. This function was already implemented by most backends, but some of them can run into llvm_unreachable which makes it impossible to recover from unknown instruction sizes. This fix returns the sentinel value ~0U instead of unreachable in the AArch64, AMDGPU, MSP430 backends.

[rovka]

lib/Target/AArch64/AArch64BranchRelaxation.cpp

174

I'm not sure I understand this comment. Why wouldn't you check after every use of getInstSizeInBytes?

176

I think you can use an assert instead (here and everywhere else).

[SjoerdMeijer]

lib/Target/AArch64/AArch64BranchRelaxation.cpp

174

I wanted to avoid exactly that (checking it after every use) because this function is used quite a lot in this file.
This function computeBlockSize sees all instructions, so actually also checking it elsewhere would be redundant?

176

I wanted to make this a non-functional change. The current/old behaviour is that we can run into a compiler abort (unreachable) for unsupported instructions. If we change unreachable into asserts, then the new behaviour for Release builds that don't have asserts is different. I.e., instead of aborting, the compiler continues its analysis with wrong numbers (it would wrap around as we add ~0U). I think that is undesired behaviour, would you agree?

[rovka]

lib/Target/AArch64/AArch64BranchRelaxation.cpp

174

Maybe it would now, but that might change someday. I think it would be safer to assert after every use.
Anyway, if you insist on doing this, you should add a similar comment to MSP420BranchSelector, where you're also only checking after the first use.

176

I'm not 100% sure about that.
In the coding standards it says: "When assertions are disabled (i.e. in release builds), llvm_unreachable becomes a hint to compilers to skip generating code for this branch. If the compiler does not support this, it will fall back to the “abort” implementation."
So, yes, we see aborts now, but I'm not convinced that's always the intention (OTOH that might be old text, or I might be misreading it).

[rovka]

lib/Target/AArch64/AArch64BranchRelaxation.cpp

174

430*

[SjoerdMeijer]

lib/Target/AArch64/AArch64BranchRelaxation.cpp

176

Alright, excellent, I am actually more than happy to use asserts because it is much cleaner! And then I will simply do it after each call.

[SjoerdMeijer]

Use asserts for checking the return value.

[arsenm]

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

338–340

Maybe add a TII->getKnownInstSizeInBytes to avoid repeating the same assertion in N places?

lib/Target/AArch64/AArch64BranchRelaxation.cpp

void AArch64BranchRelaxation::scanFunction() {
   // Compute block offsets and known bits.
   adjustBlockOffsets(*MF->begin());
 }
 
 /// computeBlockSize - Compute the size for MBB.
 /// This function updates BlockInfo directly.
 void AArch64BranchRelaxation::computeBlockSize(const MachineBasicBlock &MBB) {
   unsigned Size = 0;
-  for (const MachineInstr &MI : MBB)
-    Size += TII->getInstSizeInBytes(MI);
+  for (const MachineInstr &MI : MBB) {
+    unsigned InstSize = TII->getInstSizeInBytes(MI);
+    assert(InstSize != ~0U && "Failed to get instruction size");
+    Size += InstSize;
+  }
   BlockInfo[MBB.getNumber()].Size = Size;
 }
 
 /// getInstrOffset - Return the current offset of the specified machine
 /// instruction from the start of the function.  This offset changes as stuff is
 /// moved around inside the function.
 unsigned AArch64BranchRelaxation::getInstrOffset(const MachineInstr &MI) const {
   const MachineBasicBlock *MBB = MI.getParent();
 
   // The offset is composed of two things: the sum of the sizes of all MBB's
   // before this instruction's block, and the offset from the start of the block
   // it is in.
   unsigned Offset = BlockInfo[MBB->getNumber()].Offset;
 
   // Sum instructions before MI in MBB.
   for (MachineBasicBlock::const_iterator I = MBB->begin(); &*I != &MI; ++I) {
     assert(I != MBB->end() && "Didn't find MI in its own basic block?");
-    Offset += TII->getInstSizeInBytes(*I);
+    unsigned InstSize = TII->getInstSizeInBytes(*I);
+    assert(InstSize != ~0U && "Failed to get instruction size");
+    Offset += InstSize;
   }
 
   return Offset;
 }
 
 void AArch64BranchRelaxation::adjustBlockOffsets(MachineBasicBlock &Start) {
   unsigned PrevNum = Start.getNumber();
   for (auto &MBB : make_range(MachineFunction::iterator(Start), MF->end())) {
if (Opc == AArch64::TBZW || Opc == AArch64::TBNZW ||
       Opc == AArch64::TBZX || Opc == AArch64::TBNZX)
     MIB.addOperand(OldBr.getOperand(1));
 
   if (OldBr.getOpcode() == AArch64::Bcc)
     invertBccCondition(*MIB);
 
   MIB.addMBB(&NewDestBB);
 
-  return TII->getInstSizeInBytes(*MIB);
+  unsigned InstSize = TII->getInstSizeInBytes(*MIB);
+  assert(InstSize != ~0U && "Failed to get instruction size");
+  return InstSize;
 }
 
 /// Insert an unconditional branch at the end of \p MBB to \p DestBB.
 /// \returns the number of bytes emitted.
 unsigned AArch64BranchRelaxation::insertUnconditionalBranch(
   MachineBasicBlock &MBB,
   MachineBasicBlock &DestBB,
   const DebugLoc &DL) const {
   MachineInstr *MI = BuildMI(&MBB, DL, TII->get(AArch64::B))
     .addMBB(&DestBB);
 
-  return TII->getInstSizeInBytes(*MI);
+  unsigned InstSize = TII->getInstSizeInBytes(*MI);
+  assert(InstSize != ~0U && "Failed to get instruction size");
+  return InstSize;
 }
 
 static void changeBranchDestBlock(MachineInstr &MI,
                                   MachineBasicBlock &NewDestBB) {
   unsigned OpNum = 0;
   unsigned Opc = MI.getOpcode();
 
   if (Opc != AArch64::B) {
if (std::next(MachineBasicBlock::iterator(MI)) ==
       if (isBlockInRange(MI, *NewDest)) {
         DEBUG(dbgs() << "  Invert condition and swap its destination with "
                      << *BMI);
         changeBranchDestBlock(*BMI, *DestBB);
 
         int NewSize =
           insertInvertedConditionalBranch(*MBB, MI.getIterator(),
                                           MI.getDebugLoc(), MI, *NewDest);
-        int OldSize = TII->getInstSizeInBytes(MI);
+        unsigned OldSize = TII->getInstSizeInBytes(MI);
+        assert(OldSize != ~0U && "Failed to get instruction size");
         BlockInfo[MBB->getNumber()].Size += (NewSize - OldSize);
         MI.eraseFromParent();
         return true;
       }
     }
   }
 
   if (NeedSplit) {
     // Analyze the branch so we know how to update the successor lists.
     MachineBasicBlock *TBB, *FBB;
     SmallVector<MachineOperand, 2> Cond;
     TII->analyzeBranch(*MBB, TBB, FBB, Cond, false);
 
     MachineBasicBlock *NewBB = splitBlockBeforeInstr(MI);
     // No need for the branch to the next block. We're adding an unconditional
     // branch to the destination.
-    int delta = TII->getInstSizeInBytes(MBB->back());
+    unsigned delta = TII->getInstSizeInBytes(MBB->back());
+    assert(delta != ~0U && "Failed to get instruction size");
     BlockInfo[MBB->getNumber()].Size -= delta;
     MBB->back().eraseFromParent();
     // BlockInfo[SplitBB].Offset is wrong temporarily, fixed below
 
     // Update the successor lists according to the transformation to follow.
     // Do it here since if there's no split, no update is needed.
     MBB->replaceSuccessor(FBB, NewBB);
     NewBB->addSuccessor(FBB);
bool AArch64BranchRelaxation::fixupConditionalBranch(MachineInstr &MI) {
 
   // Insert a new conditional branch and a new unconditional branch.
   MBBSize += insertInvertedConditionalBranch(*MBB, MBB->end(),
                                              MI.getDebugLoc(), MI, NextBB);
 
   MBBSize += insertUnconditionalBranch(*MBB, *DestBB, MI.getDebugLoc());
 
   // Remove the old conditional branch.  It may or may not still be in MBB.
-  MBBSize -= TII->getInstSizeInBytes(MI);
+  unsigned InstSize = TII->getInstSizeInBytes(MI);
+  assert(InstSize != ~0U && "Failed to get instruction size");
+  MBBSize -= InstSize;
   MI.eraseFromParent();
 
   // Finally, keep the block offsets up to date.
   adjustBlockOffsets(*MBB);
   return true;
 }
 
 bool AArch64BranchRelaxation::relaxBranchInstructions() {

lib/Target/AArch64/AArch64InstrInfo.cpp

unsigned AArch64InstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   case TargetOpcode::EH_LABEL:
   case TargetOpcode::IMPLICIT_DEF:
   case TargetOpcode::KILL:
     return 0;
   case AArch64::TLSDESC_CALLSEQ:
     // This gets lowered to an instruction sequence which takes 16 bytes
     return 16;
   }
-
-  llvm_unreachable("getInstSizeInBytes()- Unable to determin insn size");
+  return ~0U;
 }
 
 static void parseCondBranch(MachineInstr *LastInst, MachineBasicBlock *&Target,
                             SmallVectorImpl<MachineOperand> &Cond) {
   // Block ends with fall-through condbranch.
   switch (LastInst->getOpcode()) {
   default:
     llvm_unreachable("Unknown branch instruction?");

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp

void AMDGPUAsmPrinter::getSIProgramInfo(SIProgramInfo &ProgInfo,
   for (const MachineBasicBlock &MBB : MF) {
     for (const MachineInstr &MI : MBB) {
       // TODO: CodeSize should account for multiple functions.
 
       // TODO: Should we count size of debug info?
       if (MI.isDebugValue())
         continue;
 
-      CodeSize += TII->getInstSizeInBytes(MI);
+      unsigned InstSize = TII->getInstSizeInBytes(MI);
+      assert(InstSize != ~0U && "Failed to get instruction size");
+      CodeSize += InstSize;
 
       unsigned numOperands = MI.getNumOperands();
       for (unsigned op_idx = 0; op_idx < numOperands; op_idx++) {
         const MachineOperand &MO = MI.getOperand(op_idx);
         unsigned width = 0;
         bool isSGPR = false;
 
         if (!MO.isReg())

lib/Target/AMDGPU/SIInstrInfo.cpp

unsigned SIInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   case TargetOpcode::EH_LABEL:
     return 0;
   case TargetOpcode::INLINEASM: {
     const MachineFunction *MF = MI.getParent()->getParent();
     const char *AsmStr = MI.getOperand(0).getSymbolName();
     return getInlineAsmLength(AsmStr, *MF->getTarget().getMCAsmInfo());
   }
   default:
-    llvm_unreachable("unable to find instruction size");
+    return ~0U;
   }
 }
 
 ArrayRef<std::pair<int, const char *>>
 SIInstrInfo::getSerializableTargetIndices() const {
   static const std::pair<int, const char *> TargetIndices[] = {
       {AMDGPU::TI_CONSTDATA_START, "amdgpu-constdata-start"},
       {AMDGPU::TI_SCRATCH_RSRC_DWORD0, "amdgpu-scratch-rsrc-dword0"},

lib/Target/MSP430/MSP430BranchSelector.cpp

bool MSP430BSel::runOnMachineFunction(MachineFunction &Fn) {
   // Give the blocks of the function a dense, in-order, numbering.
   Fn.RenumberBlocks();
   BlockSizes.resize(Fn.getNumBlockIDs());
 
   // Measure each MBB and compute a size for the entire function.
   unsigned FuncSize = 0;
   for (MachineBasicBlock &MBB : Fn) {
     unsigned BlockSize = 0;
-    for (MachineInstr &MI : MBB)
-      BlockSize += TII->getInstSizeInBytes(MI);
+    for (MachineInstr &MI : MBB) {
+      unsigned InstSize = TII->getInstSizeInBytes(MI);
+      assert(InstSize != ~0U && "Failed to get instruction size");
+      BlockSize += InstSize;
+    }
 
     BlockSizes[MBB.getNumber()] = BlockSize;
     FuncSize += BlockSize;
   }
 
   // If the entire function is smaller than the displacement of a branch field,
   // we know we don't need to shrink any branches in this function.  This is a
   // common case.
while (MadeChange) {
     for (MachineFunction::iterator MFI = Fn.begin(), E = Fn.end(); MFI != E;
          ++MFI) {
       MachineBasicBlock &MBB = *MFI;
       unsigned MBBStartOffset = 0;
       for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end();
            I != E; ++I) {
         if ((I->getOpcode() != MSP430::JCC || I->getOperand(0).isImm()) &&
             I->getOpcode() != MSP430::JMP) {
-          MBBStartOffset += TII->getInstSizeInBytes(*I);
+          unsigned InstSize = TII->getInstSizeInBytes(*I);
+          assert(InstSize != ~0U && "Failed to get instruction size");
+          MBBStartOffset += InstSize;
           continue;
         }
 
         // Determine the offset from the current branch to the destination
         // block.
         MachineBasicBlock *Dest = I->getOperand(0).getMBB();
 
         int BranchSize;

lib/Target/MSP430/MSP430InstrInfo.cpp

 /// instruction may be.  This returns the maximum number of bytes.
 ///
 unsigned MSP430InstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   const MCInstrDesc &Desc = MI.getDesc();
 
   switch (Desc.TSFlags & MSP430II::SizeMask) {
   default:
     switch (Desc.getOpcode()) {
-    default: llvm_unreachable("Unknown instruction size!");
+    default: return ~0U;
     case TargetOpcode::CFI_INSTRUCTION:
     case TargetOpcode::EH_LABEL:
     case TargetOpcode::IMPLICIT_DEF:
     case TargetOpcode::KILL:
     case TargetOpcode::DBG_VALUE:
       return 0;
     case TargetOpcode::INLINEASM: {
       const MachineFunction *MF = MI.getParent()->getParent();
       const TargetInstrInfo &TII = *MF->getSubtarget().getInstrInfo();
       return TII.getInlineAsmLength(MI.getOperand(0).getSymbolName(),
                                     *MF->getTarget().getMCAsmInfo());
     }
     }
   case MSP430II::SizeSpecial:
     switch (MI.getOpcode()) {
-    default: llvm_unreachable("Unknown instruction size!");
+    default: return ~0U;
     case MSP430::SAR8r1c:
     case MSP430::SAR16r1c:
       return 4;
     }
   case MSP430II::Size2Bytes:
     return 2;
   case MSP430II::Size4Bytes:
     return 4;
   case MSP430II::Size6Bytes:
     return 6;
   }
 }