Improve the packing of RefPosition (#12263)

The LSRA RefPosition type was not packing well. In particular,
bitfields don't pack unless they have the same base type.

Move all the data member declarations to the top, and make them adjacent,
so it is easier to see them all together. Also, change all bitfields
to "unsigned char" base type.

This reduces the size from 36 to 28 bytes for x86, and 48 to 40 bytes
for x64 (release, on Windows).

2 files changed, 103 insertions, 101 deletions

diff --git a/src/jit/lsra.cpp b/src/jit/lsra.cpp
index ceb3e53490..b3031c6537 100644
--- a/src/jit/lsra.cpp
+++ b/src/jit/lsra.cpp
@@ -825,7 +825,7 @@ RefPosition* LinearScan::newRefPosition(
     newRP->registerAssignment = mask;
 
     newRP->setMultiRegIdx(0);
-    newRP->setAllocateIfProfitable(0);
+    newRP->setAllocateIfProfitable(false);
 
     associateRefPosWithInterval(newRP);
 
@@ -912,7 +912,7 @@ RefPosition* LinearScan::newRefPosition(Interval*    theInterval,
     newRP->registerAssignment = mask;
 
     newRP->setMultiRegIdx(multiRegIdx);
-    newRP->setAllocateIfProfitable(0);
+    newRP->setAllocateIfProfitable(false);
 
 #ifdef DEBUG
     newRP->minRegCandidateCount = minRegCandidateCount;
@@ -4077,7 +4077,7 @@ void LinearScan::buildRefPositionsForNode(GenTree*                  tree,
 
         if (regOptionalAtUse)
         {
-            pos->setAllocateIfProfitable(1);
+            pos->setAllocateIfProfitable(true);
         }
     }
     JITDUMP("\n");
diff --git a/src/jit/lsra.h b/src/jit/lsra.h
index b59c808dde..3ef17f0610 100644
--- a/src/jit/lsra.h
+++ b/src/jit/lsra.h
@@ -1445,6 +1445,105 @@ public:
 class RefPosition
 {
 public:
+    // A RefPosition refers to either an Interval or a RegRecord. 'referent' points to one
+    // of these types. If it refers to a RegRecord, then 'isPhysRegRef' is true. If it
+    // refers to an Interval, then 'isPhysRegRef' is false.
+    //
+    // Q: can 'referent' be NULL?
+
+    Referenceable* referent;
+
+    // nextRefPosition is the next in code order.
+    // Note that in either case there is no need for these to be doubly linked, as they
+    // are only traversed in the forward direction, and are not moved.
+    RefPosition* nextRefPosition;
+
+    // The remaining fields are common to both options
+    GenTree*     treeNode;
+    unsigned int bbNum;
+
+    // Prior to the allocation pass, registerAssignment captures the valid registers
+    // for this RefPosition. An empty set means that any register is valid.  A non-empty
+    // set means that it must be one of the given registers (may be the full set if the
+    // only constraint is that it must reside in SOME register)
+    // After the allocation pass, this contains the actual assignment
+    LsraLocation nodeLocation;
+    regMaskTP    registerAssignment;
+
+    RefType refType;
+
+    // NOTE: C++ only packs bitfields if the base type is the same. So make all the base
+    // NOTE: types of the logically "bool" types that follow 'unsigned char', so they match
+    // NOTE: RefType that precedes this, and multiRegIdx can also match.
+
+    // Indicates whether this ref position is to be allocated a reg only if profitable. Currently these are the
+    // ref positions that lower/codegen has indicated as reg optional and is considered a contained memory operand if
+    // no reg is allocated.
+    unsigned char allocRegIfProfitable : 1;
+
+    // Used by RefTypeDef/Use positions of a multi-reg call node.
+    // Indicates the position of the register that this ref position refers to.
+    // The max bits needed is based on max value of MAX_RET_REG_COUNT value
+    // across all targets and that happens 4 on on Arm.  Hence index value
+    // would be 0..MAX_RET_REG_COUNT-1.
+    unsigned char multiRegIdx : 2;
+
+    // Last Use - this may be true for multiple RefPositions in the same Interval
+    unsigned char lastUse : 1;
+
+    // Spill and Copy info
+    //   reload indicates that the value was spilled, and must be reloaded here.
+    //   spillAfter indicates that the value is spilled here, so a spill must be added.
+    //   copyReg indicates that the value needs to be copied to a specific register,
+    //      but that it will also retain its current assigned register.
+    //   moveReg indicates that the value needs to be moved to a different register,
+    //      and that this will be its new assigned register.
+    // A RefPosition may have any flag individually or the following combinations:
+    //  - reload and spillAfter (i.e. it remains in memory), but not in combination with copyReg or moveReg
+    //    (reload cannot exist with copyReg or moveReg; it should be reloaded into the appropriate reg)
+    //  - spillAfter and copyReg (i.e. it must be copied to a new reg for use, but is then spilled)
+    //  - spillAfter and moveReg (i.e. it most be both spilled and moved)
+    //    NOTE: a moveReg involves an explicit move, and would usually not be needed for a fixed Reg if it is going
+    //    to be spilled, because the code generator will do the move to the fixed register, and doesn't need to
+    //    record the new register location as the new "home" location of the lclVar. However, if there is a conflicting
+    //    use at the same location (e.g. lclVar V1 is in rdx and needs to be in rcx, but V2 needs to be in rdx), then
+    //    we need an explicit move.
+    //  - copyReg and moveReg must not exist with each other.
+
+    unsigned char reload : 1;
+    unsigned char spillAfter : 1;
+    unsigned char copyReg : 1;
+    unsigned char moveReg : 1; // true if this var is moved to a new register
+
+    unsigned char isPhysRegRef : 1; // true if 'referent' points of a RegRecord, false if it points to an Interval
+    unsigned char isFixedRegRef : 1;
+    unsigned char isLocalDefUse : 1;
+
+    // delayRegFree indicates that the register should not be freed right away, but instead wait
+    // until the next Location after it would normally be freed.  This is used for the case of
+    // non-commutative binary operators, where op2 must not be assigned the same register as
+    // the target.  We do this by not freeing it until after the target has been defined.
+    // Another option would be to actually change the Location of the op2 use until the same
+    // Location as the def, but then it could potentially reuse a register that has been freed
+    // from the other source(s), e.g. if it's a lastUse or spilled.
+    unsigned char delayRegFree : 1;
+
+    // outOfOrder is marked on a (non-def) RefPosition that doesn't follow a definition of the
+    // register currently assigned to the Interval.  This happens when we use the assigned
+    // register from a predecessor that is not the most recently allocated BasicBlock.
+    unsigned char outOfOrder : 1;
+
+#ifdef DEBUG
+    // Minimum number registers that needs to be ensured while
+    // constraining candidates for this ref position under
+    // LSRA stress.
+    unsigned minRegCandidateCount;
+
+    // The unique RefPosition number, equal to its index in the
+    // refPositions list. Only used for debugging dumps.
+    unsigned rpNum;
+#endif // DEBUG
+
     RefPosition(unsigned int bbNum, LsraLocation nodeLocation, GenTree* treeNode, RefType refType)
         : referent(nullptr)
         , nextRefPosition(nullptr)
@@ -1471,14 +1570,6 @@ public:
     {
     }
 
-    // A RefPosition refers to either an Interval or a RegRecord. 'referent' points to one
-    // of these types. If it refers to a RegRecord, then 'isPhysRegRef' is true. If it
-    // refers to an Interval, then 'isPhysRegRef' is false.
-    //
-    // Q: can 'referent' be NULL?
-
-    Referenceable* referent;
-
     Interval* getInterval()
     {
         assert(!isPhysRegRef);
@@ -1502,23 +1593,6 @@ public:
         registerAssignment = genRegMask(r->regNum);
     }
 
-    // nextRefPosition is the next in code order.
-    // Note that in either case there is no need for these to be doubly linked, as they
-    // are only traversed in the forward direction, and are not moved.
-    RefPosition* nextRefPosition;
-
-    // The remaining fields are common to both options
-    GenTree*     treeNode;
-    unsigned int bbNum;
-
-    // Prior to the allocation pass, registerAssignment captures the valid registers
-    // for this RefPosition. An empty set means that any register is valid.  A non-empty
-    // set means that it must be one of the given registers (may be the full set if the
-    // only constraint is that it must reside in SOME register)
-    // After the allocation pass, this contains the actual assignment
-    LsraLocation nodeLocation;
-    regMaskTP    registerAssignment;
-
     regNumber assignedReg()
     {
         if (registerAssignment == RBM_NONE)
@@ -1529,8 +1603,6 @@ public:
         return genRegNumFromMask(registerAssignment);
     }
 
-    RefType refType;
-
     // Returns true if it is a reference on a gentree node.
     bool IsActualRef()
     {
@@ -1547,14 +1619,7 @@ public:
                !AllocateIfProfitable();
     }
 
-    // Indicates whether this ref position is to be allocated
-    // a reg only if profitable. Currently these are the
-    // ref positions that lower/codegen has indicated as reg
-    // optional and is considered a contained memory operand if
-    // no reg is allocated.
-    unsigned allocRegIfProfitable : 1;
-
-    void setAllocateIfProfitable(unsigned val)
+    void setAllocateIfProfitable(bool val)
     {
         allocRegIfProfitable = val;
     }
@@ -1570,13 +1635,6 @@ public:
         return allocRegIfProfitable && !copyReg && !moveReg;
     }
 
-    // Used by RefTypeDef/Use positions of a multi-reg call node.
-    // Indicates the position of the register that this ref position refers to.
-    // The max bits needed is based on max value of MAX_RET_REG_COUNT value
-    // across all targets and that happens 4 on on Arm.  Hence index value
-    // would be 0..MAX_RET_REG_COUNT-1.
-    unsigned multiRegIdx : 2;
-
     void setMultiRegIdx(unsigned idx)
     {
         multiRegIdx = idx;
@@ -1588,67 +1646,11 @@ public:
         return multiRegIdx;
     }
 
-    // Last Use - this may be true for multiple RefPositions in the same Interval
-    bool lastUse : 1;
-
-    // Spill and Copy info
-    //   reload indicates that the value was spilled, and must be reloaded here.
-    //   spillAfter indicates that the value is spilled here, so a spill must be added.
-    //   copyReg indicates that the value needs to be copied to a specific register,
-    //      but that it will also retain its current assigned register.
-    //   moveReg indicates that the value needs to be moved to a different register,
-    //      and that this will be its new assigned register.
-    // A RefPosition may have any flag individually or the following combinations:
-    //  - reload and spillAfter (i.e. it remains in memory), but not in combination with copyReg or moveReg
-    //    (reload cannot exist with copyReg or moveReg; it should be reloaded into the appropriate reg)
-    //  - spillAfter and copyReg (i.e. it must be copied to a new reg for use, but is then spilled)
-    //  - spillAfter and moveReg (i.e. it most be both spilled and moved)
-    //    NOTE: a moveReg involves an explicit move, and would usually not be needed for a fixed Reg if it is going
-    //    to be spilled, because the code generator will do the move to the fixed register, and doesn't need to
-    //    record the new register location as the new "home" location of the lclVar. However, if there is a conflicting
-    //    use at the same location (e.g. lclVar V1 is in rdx and needs to be in rcx, but V2 needs to be in rdx), then
-    //    we need an explicit move.
-    //  - copyReg and moveReg must not exist with each other.
-
-    bool reload : 1;
-    bool spillAfter : 1;
-    bool copyReg : 1;
-    bool moveReg : 1; // true if this var is moved to a new register
-
-    bool isPhysRegRef : 1; // true if 'referent' points of a RegRecord, false if it points to an Interval
-    bool isFixedRegRef : 1;
-    bool isLocalDefUse : 1;
-
-    // delayRegFree indicates that the register should not be freed right away, but instead wait
-    // until the next Location after it would normally be freed.  This is used for the case of
-    // non-commutative binary operators, where op2 must not be assigned the same register as
-    // the target.  We do this by not freeing it until after the target has been defined.
-    // Another option would be to actually change the Location of the op2 use until the same
-    // Location as the def, but then it could potentially reuse a register that has been freed
-    // from the other source(s), e.g. if it's a lastUse or spilled.
-    bool delayRegFree : 1;
-
-    // outOfOrder is marked on a (non-def) RefPosition that doesn't follow a definition of the
-    // register currently assigned to the Interval.  This happens when we use the assigned
-    // register from a predecessor that is not the most recently allocated BasicBlock.
-    bool outOfOrder : 1;
-
     LsraLocation getRefEndLocation()
     {
         return delayRegFree ? nodeLocation + 1 : nodeLocation;
     }
 
-#ifdef DEBUG
-    // Minimum number registers that needs to be ensured while
-    // constraining candidates for this ref position under
-    // LSRA stress.
-    unsigned minRegCandidateCount;
-
-    // The unique RefPosition number, equal to its index in the
-    // refPositions list. Only used for debugging dumps.
-    unsigned rpNum;
-#endif // DEBUG
-
     bool isIntervalRef()
     {
         return (!isPhysRegRef && (referent != nullptr));