[DwarfExpression] Refactor dwarf expression (NFC)

[djtodoro]

Summary

Refactor location description kind in order to be easier for extensions (needed for D60866).
In addition, cut off some bits from the other class fields.

[aprantl]

lib/CodeGen/AsmPrinter/DwarfExpression.h

125

Perhaps I'm forgetting about a use-case here, but shouldn't memory and register be mutually exclusive? If so, should they share the same bit?

[djtodoro]

-For now, all expression kinds are mutually exclusive, therefore handle it properly.

[djtodoro]

lib/CodeGen/AsmPrinter/DwarfExpression.h

125

Yes, thanks! We will need additional flags in order to mark parameter and entry value expressions (e.g. at the same time an expression will be entry value and register).

[aprantl]

lib/CodeGen/AsmPrinter/DwarfExpression.h

138

We can let the compiler do this for us by defining:

unsigned LocationKind : 3;
unsigned LocationFlags: ...

and initializing it to zero in the constructor. If we are worried about memory use here, I'd also cut off some bits from DwarfVersion.

[djtodoro]

lib/CodeGen/AsmPrinter/DwarfExpression.h

138

Good idea, thanks!

[djtodoro]

-Introduce the LokcationKind and the LokcationFlags
-Cut off bits from the DwarfVersion

[dstenb]

lib/CodeGen/AsmPrinter/DwarfExpression.h

114

I don't think this has any effect on x86 for example, since SubRegisterSizeInBits will be padded to 32 bits. Perhaps it is a non-intrusive change to move DwarfVersion down to the other bit-fields?

[djtodoro]

lib/CodeGen/AsmPrinter/DwarfExpression.h

114

I see.. I didn't think about the alignment.
I think we can let that as unsigned.

[djtodoro]

-Keep the DwarfVersion as unsigned

[aprantl]

lib/CodeGen/AsmPrinter/DwarfExpression.h

114

I think what @dstenb meant was to make this into:

uint64_t OffsetInBits = 0;
 unsigned SubRegisterOffsetInBits = 0;

 /// The kind of location description being produced.
 enum { Unknown = 0, Register, Memory, Implicit };

 unsigned LocationKind : 3;
 unsigned LocationFlags : 2; 
 unsigned DwarfVersion : 4;

this way the packed bitfields all share one 32-bit slot.

[aprantl]

lib/CodeGen/AsmPrinter/DwarfExpression.h

118

and these could be unsigned : 16.

[djtodoro]

lib/CodeGen/AsmPrinter/DwarfExpression.h

114

Sure. I understood the idea, but I didn’t want to move (and refactor) those other fields in this commit, but refactoring it this way makes sense to me. :)

I will do that way.

[djtodoro]

-Cut off some bits from the other class fields
-Rename the title
-Change the summary

[aprantl]

Thanks! One more nit inside, otherwise good to go.

lib/CodeGen/AsmPrinter/DwarfExpression.h

146

assert(SizeInBits < 65536 && OffsetInBits < 65536);

[djtodoro]

-Add an assertion when emitting pieces info about a subregister

[djtodoro]

@aprantl Thanks a lot for the review!

lib/CodeGen/AsmPrinter/DwarfExpression.h

protected:
 
   DwarfCompileUnit &CU;
 
   /// The register location, if any.
   SmallVector<Register, 2> DwarfRegs;
 
   /// Current Fragment Offset in Bits.
   uint64_t OffsetInBits = 0;
   unsigned DwarfVersion;
 
   /// Sometimes we need to add a DW_OP_bit_piece to describe a subregister.
   unsigned SubRegisterSizeInBits = 0;
   unsigned SubRegisterOffsetInBits = 0;
 
   /// The kind of location description being produced.
-  enum { Unknown = 0, Register, Memory, Implicit } LocationKind = Unknown;
+  enum { Unknown = 0, Register, Memory, Implicit };
 
+  unsigned LocationKind : 3;
+  unsigned LocationFlags : 2;
+
+public:
+  bool isUnknownLocation() const {
+    return LocationKind == Unknown;
+  }
+
+  bool isMemoryLocation() const {
+    return LocationKind == Memory;
+  }
+
+  bool isRegisterLocation() const {
+    return LocationKind == Register;
+  }
+
+  bool isImplicitLocation() const {
+    return LocationKind == Implicit;
+  }
+
+protected:
   /// Push a DW_OP_piece / DW_OP_bit_piece for emitting later, if one is needed
   /// to represent a subregister.
   void setSubRegisterPiece(unsigned SizeInBits, unsigned OffsetInBits) {
     SubRegisterSizeInBits = SizeInBits;
     SubRegisterOffsetInBits = OffsetInBits;
   }
 
   /// Add masking operations to stencil out a subregister.
   void maskSubRegister();
 
   /// Output a dwarf operand and an optional assembler comment.
protected:
   /// heuristics to disambiguate the value vs. location status of the
   /// expression.  See PR21176 for more details.
   void addStackValue();
 
   ~DwarfExpression() = default;
 
 public:
   DwarfExpression(unsigned DwarfVersion, DwarfCompileUnit &CU)
-    : CU(CU), DwarfVersion(DwarfVersion) {}
+      : CU(CU), DwarfVersion(DwarfVersion), LocationKind(Unknown),
+        LocationFlags(Unknown) {}
 
   /// This needs to be called last to commit any pending changes.
   void finalize();
 
   /// Emit a signed constant.
   void addSignedConstant(int64_t Value);
 
   /// Emit an unsigned constant.
   void addUnsignedConstant(uint64_t Value);
 
   /// Emit an unsigned constant.
   void addUnsignedConstant(const APInt &Value);
 
-  bool isMemoryLocation() const { return LocationKind == Memory; }
-  bool isUnknownLocation() const { return LocationKind == Unknown; }
-
   /// Lock this down to become a memory location description.
   void setMemoryLocationKind() {
-    assert(LocationKind == Unknown);
+    assert(isUnknownLocation());
     LocationKind = Memory;
   }
 
   /// Emit a machine register location. As an optimization this may also consume
   /// the prefix of a DwarfExpression if a more efficient representation for
   /// combining the register location and the first operation exists.
   ///
   /// \param FragmentOffsetInBits     If this is one fragment out of a

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

void DwarfExpression::emitConstu(uint64_t Value) {
   } else {
     emitOp(dwarf::DW_OP_constu);
     emitUnsigned(Value);
   }
 }
 
 void DwarfExpression::addReg(int DwarfReg, const char *Comment) {
  assert(DwarfReg >= 0 && "invalid negative dwarf register number");
- assert((LocationKind == Unknown || LocationKind == Register) &&
+ assert((isUnknownLocation() || isRegisterLocation()) &&
         "location description already locked down");
  LocationKind = Register;
  if (DwarfReg < 32) {
    emitOp(dwarf::DW_OP_reg0 + DwarfReg, Comment);
   } else {
     emitOp(dwarf::DW_OP_regx, Comment);
     emitUnsigned(DwarfReg);
   }
 }
 
 void DwarfExpression::addBReg(int DwarfReg, int Offset) {
   assert(DwarfReg >= 0 && "invalid negative dwarf register number");
-  assert(LocationKind != Register && "location description already locked down");
+  assert(!isRegisterLocation() && "location description already locked down");
   if (DwarfReg < 32) {
     emitOp(dwarf::DW_OP_breg0 + DwarfReg);
   } else {
     emitOp(dwarf::DW_OP_bregx);
     emitUnsigned(DwarfReg);
   }
   emitSigned(Offset);
 }

 }
 
 void DwarfExpression::addStackValue() {
   if (DwarfVersion >= 4)
     emitOp(dwarf::DW_OP_stack_value);
 }
 
 void DwarfExpression::addSignedConstant(int64_t Value) {
-  assert(LocationKind == Implicit || LocationKind == Unknown);
+  assert(isImplicitLocation() || isUnknownLocation());
   LocationKind = Implicit;
   emitOp(dwarf::DW_OP_consts);
   emitSigned(Value);
 }
 
 void DwarfExpression::addUnsignedConstant(uint64_t Value) {
-  assert(LocationKind == Implicit || LocationKind == Unknown);
+  assert(isImplicitLocation() || isUnknownLocation());
   LocationKind = Implicit;
   emitConstu(Value);
 }
 
 void DwarfExpression::addUnsignedConstant(const APInt &Value) {
-  assert(LocationKind == Implicit || LocationKind == Unknown);
+  assert(isImplicitLocation() || isUnknownLocation());
   LocationKind = Implicit;
 
   unsigned Size = Value.getBitWidth();
   const uint64_t *Data = Value.getRawData();
 
   // Chop it up into 64-bit pieces, because that's the maximum that
   // addUnsignedConstant takes.
   unsigned Offset = 0;
bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
   // operation to multiple DW_OP_pieces.
   if (HasComplexExpression && DwarfRegs.size() > 1) {
     DwarfRegs.clear();
     LocationKind = Unknown;
     return false;
   }
 
   // Handle simple register locations.
-  if (LocationKind != Memory && !HasComplexExpression) {
+  if (!isMemoryLocation() && !HasComplexExpression) {
     for (auto &Reg : DwarfRegs) {
       if (Reg.DwarfRegNo >= 0)
         addReg(Reg.DwarfRegNo, Reg.Comment);
       addOpPiece(Reg.Size);
     }
     DwarfRegs.clear();
     return true;
   }
case dwarf::DW_OP_LLVM_fragment: {
       SizeInBits -= OffsetInBits - FragmentOffset;
 
       // If addMachineReg requested a DW_OP_bit_piece to stencil out a
       // sub-register that is smaller than the current fragment's size, use it.
       if (SubRegisterSizeInBits)
         SizeInBits = std::min<unsigned>(SizeInBits, SubRegisterSizeInBits);
 
       // Emit a DW_OP_stack_value for implicit location descriptions.
-      if (LocationKind == Implicit)
+      if (isImplicitLocation())
         addStackValue();
 
       // Emit the DW_OP_piece.
       addOpPiece(SizeInBits, SubRegisterOffsetInBits);
       setSubRegisterPiece(0, 0);
       // Reset the location description kind.
       LocationKind = Unknown;
       return;
     }
     case dwarf::DW_OP_plus_uconst:
-      assert(LocationKind != Register);
+      assert(!isRegisterLocation());
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
       break;
     case dwarf::DW_OP_plus:
     case dwarf::DW_OP_minus:
     case dwarf::DW_OP_mul:
     case dwarf::DW_OP_div:
     case dwarf::DW_OP_mod:
     case dwarf::DW_OP_or:
     case dwarf::DW_OP_and:
     case dwarf::DW_OP_xor:
     case dwarf::DW_OP_shl:
     case dwarf::DW_OP_shr:
     case dwarf::DW_OP_shra:
     case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
       emitOp(Op->getOp());
       break;
     case dwarf::DW_OP_deref:
-      assert(LocationKind != Register);
-      if (LocationKind != Memory && ::isMemoryLocation(ExprCursor))
+      assert(!isRegisterLocation());
+      if (!isMemoryLocation() && ::isMemoryLocation(ExprCursor))
         // Turning this into a memory location description makes the deref
         // implicit.
         LocationKind = Memory;
       else
         emitOp(dwarf::DW_OP_deref);
       break;
     case dwarf::DW_OP_constu:
-      assert(LocationKind != Register);
+      assert(!isRegisterLocation());
       emitConstu(Op->getArg(0));
       break;
     case dwarf::DW_OP_LLVM_convert: {
       unsigned BitSize = Op->getArg(0);
       dwarf::TypeKind Encoding = static_cast<dwarf::TypeKind>(Op->getArg(1));
       if (DwarfVersion >= 5) {
         emitOp(dwarf::DW_OP_convert);
         // Reuse the base_type if we already have one in this CU otherwise we
case dwarf::DW_OP_LLVM_convert: {
         }
       }
       break;
     }
     case dwarf::DW_OP_stack_value:
       LocationKind = Implicit;
       break;
     case dwarf::DW_OP_swap:
-      assert(LocationKind != Register);
+      assert(!isRegisterLocation());
       emitOp(dwarf::DW_OP_swap);
       break;
     case dwarf::DW_OP_xderef:
-      assert(LocationKind != Register);
+      assert(!isRegisterLocation());
       emitOp(dwarf::DW_OP_xderef);
       break;
     case dwarf::DW_OP_deref_size:
       emitOp(dwarf::DW_OP_deref_size);
       emitData1(Op->getArg(0));
       break;
     default:
       llvm_unreachable("unhandled opcode found in expression");
     }
   }
 
-  if (LocationKind == Implicit)
+  if (isImplicitLocation())
     // Turn this into an implicit location description.
     addStackValue();
 }
 
 /// add masking operations to stencil out a subregister.
 void DwarfExpression::maskSubRegister() {
   assert(SubRegisterSizeInBits && "no subregister was registered");
   if (SubRegisterOffsetInBits > 0)