[MachineInst] Bump NumOperands back up to 24bits

[jroelofs]

Summary

In https://reviews.llvm.org/D149445, it was lowered from 32 to 16bits, which broke an internal project of ours. The relevant code being compiled is a fairly large nested switch that results in a PHI node with 65k+ operands, which can't easily be turned into a table for perf reasons.

This change unifies NumOperands, Flags, and AsmPrinterFlags into a packed 7-byte struct, which CapOperands can follow as the 8th byte, rounding it up to a nice alignment before the Info field.

rdar://111217742&109362033

[xbolva00]

Looks fine

llvm/lib/CodeGen/MachineInstr.cpp

196–197

formatting

[jroelofs]

git-clang-format'd the patch

[jroelofs]

clarify assert strings

[nikic]

llvm/include/llvm/CodeGen/MachineInstr.h

137

Why do we need the packed struct wrapper here? Doesn't defining these bitfields directly in the class work?

[jroelofs]

llvm/include/llvm/CodeGen/MachineInstr.h

137

The two 24-bit fields need to be part of a packed struct, otherwise we get some padding [1]. We can either do that around the whole struct, or just these 56 bits. I don't have a strong opinion either way.

1: https://clang.godbolt.org/z/4Kxs4fKc7

[xbolva00]

whole struct would be probably better, less churn. Plus, can we static_assert that size is <= XY?

[nikic]

llvm/include/llvm/CodeGen/MachineInstr.h

137

Can you avoid the need for that by reordering the fields to be 24 8 24 8? E.g. Flags, AsmPrinterFlags, NumOperands, CapOperands.

[jroelofs]

llvm/include/llvm/CodeGen/MachineInstr.h

137

Good eye! Yeah, that works: https://clang.godbolt.org/z/q1onMv94e

[craig.topper]

llvm/include/llvm/CodeGen/MachineInstr.h

137

If I remember right, doesn't MSVC pack bit fields differently than clang/gcc?

[nikic]

llvm/include/llvm/CodeGen/MachineInstr.h

137

Yes, for this to work on MSVC we would need all of these fields to have the same type (with different widths). However, MSVC does not produce correct layout even with packed structs. It ends up with a 34 byte struct (rather than 32 or 40), so I can only assume we'll get a terrible unaligned mess if we do that.

So I think we should just accept the size increase on MSVC.

385

These can use isUInt<LLVM_MI_FLAGS_BITS>(Flag) etc. (Though maybe not worth including the header if it's not already used.)

[jroelofs]

llvm:isUInt<>

[nikic]

LGTM

[jroelofs]

llvm/include/llvm/CodeGen/MachineInstr.h

137

Does MSVC have an analog to -fdump-record-layouts?

385

Thanks for the pointer. ArrayRecycler.h pulls it in, so it's low overhead to use it here.

llvm/include/llvm/CodeGen/MachineInstr.h

 #include "llvm/CodeGen/MachineMemOperand.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/TargetOpcodes.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/IR/InlineAsm.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/ArrayRecycler.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/TrailingObjects.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <utility>
 
 namespace llvm {
 
public:
   };
 
 private:
   const MCInstrDesc *MCID;              // Instruction descriptor.
   MachineBasicBlock *Parent = nullptr;  // Pointer to the owning basic block.
 
   // Operands are allocated by an ArrayRecycler.
   MachineOperand *Operands = nullptr;   // Pointer to the first operand.
-  uint32_t Flags = 0;                   // Various bits of additional
-                                        // information about machine
-                                        // instruction.
-  uint16_t NumOperands = 0;             // Number of operands on instruction.
-  uint8_t AsmPrinterFlags = 0;          // Various bits of information used by
-                                        // the AsmPrinter to emit helpful
-                                        // comments.  This is *not* semantic
-                                        // information.  Do not use this for
-                                        // anything other than to convey comment
-                                        // information to AsmPrinter.
 
-  // OperandCapacity has uint8_t size, so it should be next to AsmPrinterFlags
+#define LLVM_MI_NUMOPERANDS_BITS 24
+#define LLVM_MI_FLAGS_BITS 24
+#define LLVM_MI_ASMPRINTERFLAGS_BITS 8
+
+  /// Number of operands on instruction.
+  uint32_t NumOperands : LLVM_MI_NUMOPERANDS_BITS;
+
+  // OperandCapacity has uint8_t size, so it should be next to NumOperands
   // to properly pack.
   using OperandCapacity = ArrayRecycler<MachineOperand>::Capacity;
   OperandCapacity CapOperands;          // Capacity of the Operands array.
 
+  /// Various bits of additional information about the machine instruction.
+  uint32_t Flags : LLVM_MI_FLAGS_BITS;
+
+  /// Various bits of information used by the AsmPrinter to emit helpful
+  /// comments.  This is *not* semantic information.  Do not use this for
+  /// anything other than to convey comment information to AsmPrinter.
+  uint8_t AsmPrinterFlags : LLVM_MI_ASMPRINTERFLAGS_BITS;
+
   /// Internal implementation detail class that provides out-of-line storage for
   /// extra info used by the machine instruction when this info cannot be stored
   /// in-line within the instruction itself.
   ///
   /// This has to be defined eagerly due to the implementation constraints of
   /// `PointerSumType` where it is used.
   class ExtraInfo final : TrailingObjects<ExtraInfo, MachineMemOperand *,
                                           MCSymbol *, MDNode *, uint32_t> {
public:
   /// Return the asm printer flags bitvector.
   uint8_t getAsmPrinterFlags() const { return AsmPrinterFlags; }
 
   /// Clear the AsmPrinter bitvector.
   void clearAsmPrinterFlags() { AsmPrinterFlags = 0; }
 
   /// Return whether an AsmPrinter flag is set.
   bool getAsmPrinterFlag(CommentFlag Flag) const {
+    assert(isUInt<LLVM_MI_ASMPRINTERFLAGS_BITS>(unsigned(Flag)) &&
+           "Flag is out of range for the AsmPrinterFlags field");
     return AsmPrinterFlags & Flag;
   }
 
   /// Set a flag for the AsmPrinter.
   void setAsmPrinterFlag(uint8_t Flag) {
+    assert(isUInt<LLVM_MI_ASMPRINTERFLAGS_BITS>(unsigned(Flag)) &&
+           "Flag is out of range for the AsmPrinterFlags field");
     AsmPrinterFlags |= Flag;
   }
 
   /// Clear specific AsmPrinter flags.
   void clearAsmPrinterFlag(CommentFlag Flag) {
+    assert(isUInt<LLVM_MI_ASMPRINTERFLAGS_BITS>(unsigned(Flag)) &&
+           "Flag is out of range for the AsmPrinterFlags field");
     AsmPrinterFlags &= ~Flag;
   }
 
   /// Return the MI flags bitvector.
   uint32_t getFlags() const {
     return Flags;
   }
 
   /// Return whether an MI flag is set.
   bool getFlag(MIFlag Flag) const {
+    assert(isUInt<LLVM_MI_FLAGS_BITS>(unsigned(Flag)) &&
+           "Flag is out of range for the Flags field");
     return Flags & Flag;
   }
 
   /// Set a MI flag.
   void setFlag(MIFlag Flag) {
+    assert(isUInt<LLVM_MI_FLAGS_BITS>(unsigned(Flag)) &&
+           "Flag is out of range for the Flags field");
     Flags |= (uint32_t)Flag;
   }
 
   void setFlags(unsigned flags) {
+    assert(isUInt<LLVM_MI_FLAGS_BITS>(flags) &&
+           "flags to be set are out of range for the Flags field");
     // Filter out the automatically maintained flags.
     unsigned Mask = BundledPred | BundledSucc;
     Flags = (Flags & Mask) | (flags & ~Mask);
   }
 
   /// clearFlag - Clear a MI flag.
   void clearFlag(MIFlag Flag) {
+    assert(isUInt<LLVM_MI_FLAGS_BITS>(unsigned(Flag)) &&
+           "Flag to clear is out of range for the Flags field");
     Flags &= ~((uint32_t)Flag);
   }
 
   /// Return true if MI is in a bundle (but not the first MI in a bundle).
   ///
   /// A bundle looks like this before it's finalized:
   ///   ----------------
   ///   |      MI      |

llvm/lib/CodeGen/MachineInstr.cpp

for (MCPhysReg ImpUse : MCID->implicit_uses())
     addOperand(MF, MachineOperand::CreateReg(ImpUse, false, true));
 }
 
 /// MachineInstr ctor - This constructor creates a MachineInstr and adds the
 /// implicit operands. It reserves space for the number of operands specified by
 /// the MCInstrDesc.
 MachineInstr::MachineInstr(MachineFunction &MF, const MCInstrDesc &TID,
                            DebugLoc DL, bool NoImp)
-    : MCID(&TID), DbgLoc(std::move(DL)), DebugInstrNum(0) {
+    : MCID(&TID), NumOperands(0), Flags(0), AsmPrinterFlags(0),
+      DbgLoc(std::move(DL)), DebugInstrNum(0) {
   assert(DbgLoc.hasTrivialDestructor() && "Expected trivial destructor");
 
   // Reserve space for the expected number of operands.
   if (unsigned NumOps = MCID->getNumOperands() + MCID->implicit_defs().size() +
                         MCID->implicit_uses().size()) {
     CapOperands = OperandCapacity::get(NumOps);
     Operands = MF.allocateOperandArray(CapOperands);
   }
 
   if (!NoImp)
     addImplicitDefUseOperands(MF);
 }
 
 /// MachineInstr ctor - Copies MachineInstr arg exactly.
 /// Does not copy the number from debug instruction numbering, to preserve
 /// uniqueness.
 MachineInstr::MachineInstr(MachineFunction &MF, const MachineInstr &MI)
-    : MCID(&MI.getDesc()), Info(MI.Info), DbgLoc(MI.getDebugLoc()),
-      DebugInstrNum(0) {
+    : MCID(&MI.getDesc()), NumOperands(0), Flags(0), AsmPrinterFlags(0),
+      Info(MI.Info), DbgLoc(MI.getDebugLoc()), DebugInstrNum(0) {
   assert(DbgLoc.hasTrivialDestructor() && "Expected trivial destructor");
 
   CapOperands = OperandCapacity::get(MI.getNumOperands());
   Operands = MF.allocateOperandArray(CapOperands);
 
   // Copy operands.
   for (const MachineOperand &MO : MI.operands())
     addOperand(MF, MO);
static void moveOperands(MachineOperand *Dst, MachineOperand *Src,
   std::memmove(Dst, Src, NumOps * sizeof(MachineOperand));
 }
 
 /// addOperand - Add the specified operand to the instruction.  If it is an
 /// implicit operand, it is added to the end of the operand list.  If it is
 /// an explicit operand it is added at the end of the explicit operand list
 /// (before the first implicit operand).
 void MachineInstr::addOperand(MachineFunction &MF, const MachineOperand &Op) {
-  assert(NumOperands < USHRT_MAX && "Cannot add more operands.");
+  assert(isUInt<LLVM_MI_NUMOPERANDS_BITS>(NumOperands + 1) &&
+         "Cannot add more operands.");
   assert(MCID && "Cannot add operands before providing an instr descriptor");
 
   // Check if we're adding one of our existing operands.
   if (&Op >= Operands && &Op < Operands + NumOperands) {
     // This is unusual: MI->addOperand(MI->getOperand(i)).
     // If adding Op requires reallocating or moving existing operands around,
     // the Op reference could go stale. Support it by copying Op.
     MachineOperand CopyOp(Op);