[MCInstrDesc] [TableGen] Reduce size of MCOperandInfo instances

[Paul-C-Anagnostopoulos]

Summary

This patch reduces the size of the MCOperandInfo instances. The Constraints member is reduced to 16 bits, since it contains only two values of 5 bits each. This leaves room for one more constraint before the member needs to be enlarged to 32 bits.

The next step is to reduce the size of MCInstrDesc instances. But that means changing pointers to indexes, which might get some pushback.

[craig.topper]

llvm/include/llvm/MC/MCInstrDesc.h

43

What is "bit" here? There's no data associated with EARLY_CLOBBER. The original code just did (1 << MCOI::EARLY_CLOBBER) to set the flag, there was nothing in the upper bits.

llvm/utils/TableGen/InstrInfoEmitter.cpp

186

Drop commented out code?

[Paul-C-Anagnostopoulos]

Cleaned out the commented-out code.

I will auto-LGTM this early next week.

[Paul-C-Anagnostopoulos]

Let's try this again.

[craig.topper]

I don't think auto-LGTM is consistent with LLVM's code review policy. https://llvm.org/docs/CodeReview.html#code-review-workflow @dblaikie raised this on another review recently https://reviews.llvm.org/D92514

llvm/include/llvm/MC/MCInstrDesc.h

43

What about this?

[Paul-C-Anagnostopoulos]

I've been doing it for documentation changes, commenting, and certain other relatively trivial changes, That was suggested by @lattner, not decided unilaterally.

You are right, though, that this is not trivial, so I withdraw my auto-LGTM comment.

llvm/include/llvm/MC/MCInstrDesc.h

43

All the calls to get the EARLY_CLOBBER constraint only check for -1, the value returned when the constraint is not present. So it's a boolean, but based on its presence rather than its value. That seemed odd to me, so my plan is to change the calls to check for =1 rather than !=-1.

I could be convinced that presence/absence is a perfectly good boolean.

llvm/utils/TableGen/InstrInfoEmitter.cpp

186

Oh yes, I will clean out the old code.

[Paul-C-Anagnostopoulos]

llvm/include/llvm/MC/MCInstrDesc.h

43

I was going to wait to make the =1 change until after this is pushed. Do you think I should do it now?

[craig.topper]

llvm/include/llvm/MC/MCInstrDesc.h

43

I think I must have wrote my comment at the same time you were writing yours.

I think this patch should keep just (1 << MCOI::EARLY_CLOBBER) and not have the bit argument. That would be consistent with what tablegen is doing before this patch.

[Paul-C-Anagnostopoulos]

llvm/include/llvm/MC/MCInstrDesc.h

43

I will do that, but with the MCOI_EARLY_CLOBBER macro. That keeps all the weird bit field positions, sizes, and shifting in the header file.

[Paul-C-Anagnostopoulos]

This comment was removed by Paul-C-Anagnostopoulos.

[Paul-C-Anagnostopoulos]

Change early clobber so its presence is all that matters.

[craig.topper]

LGTM, thanks!

llvm/include/llvm/MC/MCInstrDesc.h

 
 class MCInst;
 
 //===----------------------------------------------------------------------===//
 // Machine Operand Flags and Description
 //===----------------------------------------------------------------------===//
 
 namespace MCOI {
-// Operand constraints
+/// Operand constraints. These are encoded in 16 bits with one of the
+/// low-order 3 bits specifying that a constraint is present and the
+/// corresponding high-order hex digit specifying the constraint value.
+/// This allows for a maximum of 3 constraints.
 enum OperandConstraint {
-  TIED_TO = 0,  // Must be allocated the same register as.
-  EARLY_CLOBBER // Operand is an early clobber register operand
+  TIED_TO = 0,  // Must be allocated the same register as specified value.
+  EARLY_CLOBBER // If present, operand is an early clobber register.
 };
 
+// Define a macro to produce each constraint value.
+#define MCOI_TIED_TO(op) \
+  ((1 << MCOI::TIED_TO) | ((op) << (4 + MCOI::TIED_TO * 4)))
+
+#define MCOI_EARLY_CLOBBER \
+  (1 << MCOI::EARLY_CLOBBER)
+
 /// These are flags set on operands, but should be considered
 /// private, all access should go through the MCOperandInfo accessors.
 /// See the accessors for a description of what these are.
 enum OperandFlags {
   LookupPtrRegClass = 0,
   Predicate,
   OptionalDef,
   BranchTarget
public:
   /// get a dynamic register class.
   int16_t RegClass;
 
   /// These are flags from the MCOI::OperandFlags enum.
   uint8_t Flags;
 
   /// Information about the type of the operand.
   uint8_t OperandType;
-  /// The lower 16 bits are used to specify which constraints are set.
-  /// The higher 16 bits are used to specify the value of constraints (4 bits
-  /// each).
-  uint32_t Constraints;
+
+  /// Operand constraints (see OperandConstraint enum).
+  uint16_t Constraints;
 
   /// Set if this operand is a pointer value and it requires a callback
   /// to look up its register class.
   bool isLookupPtrRegClass() const {
     return Flags & (1 << MCOI::LookupPtrRegClass);
   }
 
   /// Set if this is one of the operands that made up of the predicate
public:
   unsigned char Size;            // Number of bytes in encoding.
   unsigned short SchedClass;     // enum identifying instr sched class
   uint64_t Flags;                // Flags identifying machine instr class
   uint64_t TSFlags;              // Target Specific Flag values
   const MCPhysReg *ImplicitUses; // Registers implicitly read by this instr
   const MCPhysReg *ImplicitDefs; // Registers implicitly defined by this instr
   const MCOperandInfo *OpInfo;   // 'NumOperands' entries about operands
 
-  /// Returns the value of the specific constraint if
-  /// it is set. Returns -1 if it is not set.
+  /// Returns the value of the specified operand constraint if
+  /// it is present. Returns -1 if it is not present.
   int getOperandConstraint(unsigned OpNum,
                            MCOI::OperandConstraint Constraint) const {
     if (OpNum < NumOperands &&
         (OpInfo[OpNum].Constraints & (1 << Constraint))) {
-      unsigned Pos = 16 + Constraint * 4;
-      return (int)(OpInfo[OpNum].Constraints >> Pos) & 0xf;
+      unsigned ValuePos = 4 + Constraint * 4;
+      return (int)(OpInfo[OpNum].Constraints >> ValuePos) & 0x0f;
     }
     return -1;
   }
 
   /// Return the opcode number for this descriptor.
   unsigned getOpcode() const { return Opcode; }
 
   /// Return the number of declared MachineOperands for this

llvm/utils/TableGen/InstrInfoEmitter.cpp

for (unsigned j = 0, e = OperandList.size(); j != e; ++j) {
       // Fill in constraint info.
       Res += ", ";
 
       const CGIOperandList::ConstraintInfo &Constraint =
         Op.Constraints[j];
       if (Constraint.isNone())
         Res += "0";
       else if (Constraint.isEarlyClobber())
-        Res += "(1 << MCOI::EARLY_CLOBBER)";
+        Res += "MCOI_EARLY_CLOBBER";
       else {
         assert(Constraint.isTied());
-        Res += "((" + utostr(Constraint.getTiedOperand()) +
-                    " << 16) | (1 << MCOI::TIED_TO))";
+        Res += "MCOI_TIED_TO(" + utostr(Constraint.getTiedOperand()) + ")";
       }
 
       Result.push_back(Res);
     }
   }
 
   return Result;
 }