[AsmPrinter] Add generic support for verifying instruction sizes

[nikic]

Many backends rely on TII reporting correct instruction sizes for MIR level branch relaxation passes. Reporting a too small size can result in MC fixup failures (or silent miscompiles for unvalidated fixups).

Some time ago I added validation to the PPC asm printer to verify that the TII instruction size matches the actually emitted size. This was very helpful to systematically fix all incorrectly reported instruction sizes.

However, the same problem also exists in lots of other backends, so this moves the validation into AsmPrinter, controlled by a new shouldVerifyInstSize() hook in TII, which is disabled by default.

The intention here is to gradually enable this validation for more backends (which requires fixing them first).

[llvmbot]

@llvm/pr-subscribers-backend-powerpc

Author: Nikita Popov (nikic)

Changes

Many backends rely on TII reporting correct instruction sizes for MIR level branch relaxation passes. Reporting a too small size can result in MC fixup failures (or silent miscompiles for unvalidated fixups).

Some time ago I added validation to the PPC asm printer to verify that the TII instruction size matches the actually emitted size. This was very helpful to systematically fix all incorrectly reported instruction sizes.

However, the same problem also exists in lots of other backens, so this moves the validation into AsmPrinter, controlled by a new shouldVerifyInstSize() hook in TII, which is disabled by default.

The intention here is to gradually enable this validation for more backends (which requires fixing them first).

---

Full diff: https://github.com/llvm/llvm-project/pull/187703.diff

5 Files Affected:

• (modified) llvm/include/llvm/CodeGen/TargetInstrInfo.h (+6)
• (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+28)
• (modified) llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp (-26)
• (modified) llvm/lib/Target/PowerPC/PPCInstrInfo.cpp (+5)
• (modified) llvm/lib/Target/PowerPC/PPCInstrInfo.h (+2)

diff --git a/llvm/include/llvm/CodeGen/TargetInstrInfo.h b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
index 3910e77d13de7..9affa66040caa 100644
--- a/llvm/include/llvm/CodeGen/TargetInstrInfo.h
+++ b/llvm/include/llvm/CodeGen/TargetInstrInfo.h
@@ -428,6 +428,12 @@ class LLVM_ABI TargetInstrInfo : public MCInstrInfo {
     return ~0U;
   }
 
+  /// Whether the correctness of the instruction size returned by
+  /// getInstSizeInBytes() should be verified.
+  virtual bool shouldVerifyInstSize(const MachineInstr &MI) const {
+    return false;
+  }
+
   /// Return true if the instruction is as cheap as a move instruction.
   ///
   /// Targets for different archs need to override this, and different
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index db4fc5888f6d4..322dc9d8fb92c 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -2147,6 +2147,11 @@ void AsmPrinter::emitFunctionBody() {
       if (isVerbose())
         emitComments(MI, STI, OutStreamer->getCommentOS());
 
+#ifndef NDEBUG
+      MCFragment *OldFragment = OutStreamer->getCurrentFragment();
+      size_t OldFragSize = OldFragment->getFixedSize();
+#endif
+
       switch (MI.getOpcode()) {
       case TargetOpcode::CFI_INSTRUCTION:
         emitCFIInstruction(MI);
@@ -2265,6 +2270,29 @@ void AsmPrinter::emitFunctionBody() {
         break;
       }
 
+#ifndef NDEBUG
+      // Verify that the instruction size reported by InstrInfo matches the
+      // actually emitted size. Many backends performing branch relaxation
+      // on the MIR level rely on this for correctness.
+      if (OutStreamer->isObj()) {
+        const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
+        MCFragment *NewFragment = OutStreamer->getCurrentFragment();
+        // Don't try to handle fragment splitting cases.
+        if (NewFragment == OldFragment && TII->shouldVerifyInstSize(MI)) {
+          unsigned ExpectedSize = TII->getInstSizeInBytes(MI);
+          unsigned ActualSize = NewFragment->getFixedSize() - OldFragSize;
+          // FIXME: InstrInfo currently over-estimates the size of STACKMAP.
+          if (ActualSize != ExpectedSize &&
+              MI.getOpcode() != TargetOpcode::STACKMAP) {
+            dbgs() << "Size mismatch for: " << MI << "\n";
+            dbgs() << "Expected size: " << ExpectedSize << "\n";
+            dbgs() << "Actual size: " << ActualSize << "\n";
+            abort();
+          }
+        }
+      }
+#endif
+
       if (MI.isCall()) {
         if (MF->getTarget().Options.BBAddrMap)
           OutStreamer->emitLabel(createCallsiteEndSymbol(MBB));
diff --git a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
index f5d56c1e8e264..6d1ee6bb111fe 100644
--- a/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
+++ b/llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
@@ -27,7 +27,6 @@
 #include "PPCTargetMachine.h"
 #include "TargetInfo/PowerPCTargetInfo.h"
 #include "llvm/ADT/MapVector.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
@@ -946,31 +945,6 @@ void PPCAsmPrinter::emitInstruction(const MachineInstr *MI) {
     return PPC::S_None;
   };
 
-#ifndef NDEBUG
-  // Instruction sizes must be correct for PPCBranchSelector to pick the
-  // right branch kind. Verify that the reported sizes and the actually
-  // emitted sizes match.
-  unsigned ExpectedSize = Subtarget->getInstrInfo()->getInstSizeInBytes(*MI);
-  MCFragment *OldFragment = OutStreamer->getCurrentFragment();
-  size_t OldFragSize = OldFragment->getFixedSize();
-  scope_exit VerifyInstSize([&]() {
-    if (!OutStreamer->isObj())
-      return; // Can only verify size when streaming to object.
-    MCFragment *NewFragment = OutStreamer->getCurrentFragment();
-    if (NewFragment != OldFragment)
-      return; // Don't try to handle fragment splitting cases.
-    unsigned ActualSize = NewFragment->getFixedSize() - OldFragSize;
-    // FIXME: InstrInfo currently over-estimates the size of STACKMAP.
-    if (ActualSize != ExpectedSize &&
-        MI->getOpcode() != TargetOpcode::STACKMAP) {
-      dbgs() << "Size mismatch for: " << *MI << "\n";
-      dbgs() << "Expected size: " << ExpectedSize << "\n";
-      dbgs() << "Actual size: " << ActualSize << "\n";
-      abort();
-    }
-  });
-#endif
-
   // Lower multi-instruction pseudo operations.
   switch (MI->getOpcode()) {
   default: break;
diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
index 278fd37d4c229..e9a89930bb8d7 100644
--- a/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
+++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
@@ -3034,6 +3034,11 @@ unsigned PPCInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   }
 }
 
+bool PPCInstrInfo::shouldVerifyInstSize(const MachineInstr &MI) const {
+  // FIXME: The size of STACKMAP is currently over-estimated.
+  return MI.getOpcode() != TargetOpcode::STACKMAP;
+}
+
 std::pair<unsigned, unsigned>
 PPCInstrInfo::decomposeMachineOperandsTargetFlags(unsigned TF) const {
   // PPC always uses a direct mask.
diff --git a/llvm/lib/Target/PowerPC/PPCInstrInfo.h b/llvm/lib/Target/PowerPC/PPCInstrInfo.h
index 9fe5706e7b1e0..320cc94bebfd7 100644
--- a/llvm/lib/Target/PowerPC/PPCInstrInfo.h
+++ b/llvm/lib/Target/PowerPC/PPCInstrInfo.h
@@ -700,6 +700,8 @@ class PPCInstrInfo : public PPCGenInstrInfo {
   ///
   unsigned getInstSizeInBytes(const MachineInstr &MI) const override;
 
+  bool shouldVerifyInstSize(const MachineInstr &MI) const override;
+
   MCInst getNop() const override;
 
   std::pair<unsigned, unsigned>

[arsenm]

getInstSizeInBytes should always return a conservative overestimate. It cannot be expected to give an exact match in all cases

[arsenm]

That's at least true with pseudos. It's fuzzier at this point in the emission. For AMDGPU have some cases where we should be able to reduce the instruction size after relaxation determines the exact constants, we just happen to not perform that optimization today

[nikic]

True, but for the purposes of this verification, I do think being able to check the exact size is quite useful. After all, for most instructions (even most pseudos) you can determine the exact size, and it's useful to check that.

I've updated the hook to return a InstSizeVerifyMode instead that allows picking between NoVerify, ExactSize and AllowOverEstimate. In the future, I'd like to enable AllowOverEstimate by default, but a backend can use ExactSize to get stricter validation (e.g. for PPC it holds for everything apart from stackmap).

[MaskRay]

Unfamiliar with STACKMAP, do you think the over-estimation is a general problem instead of a STACKMAP specific issue?

[MaskRay]

Consider also print the section name , sth like NewFragment->getParent()->getName().

[arsenm]

Suggested change

	dbgs() << "Size mismatch for: " << MI << "\n";
	dbgs() << "Size mismatch for: " << MI;

This already includes the newline