Defer composing subregisters with DW_OP_piece

[aprantl]

Summary

This NFC(ish*) patch defers the decision on which DW_OP_pieces to emit when composing a superregister out of subregisters to addMachineRegExpression. This is in preparation of https://reviews.llvm.org/D73283.

This patch also removes the now redundant MaxSize parameter.

*) One could construct a target in which a superregister could not be fully covered by subregsiters, in which case this code gives up instead of emitting an undefined DW_OP_piece.

[aprantl]

Factored out the NFC bits.

[aprantl]

... and I updated the wrong review again.

[bjope]

Thanks for the NFC split!
I was a little bit confused at first (about "Diff 241236") but it just seems like you have uploaded the reverse diff (so the new code is to the left and the old code is to the right when looking at the diff in Phabricator).

One worry is that for input like this

DBG_VALUE $reg128bits, $noreg, !7, !DIExpression(DW_OP_LLVM_fragment, 0, 32)

where reg128bits would be the composition of four 32-bit registers on some imaginary target,
then we used to set MaxSize=32 and then we could return true from addMachineReg after finding the first subregister (covering the least significant bits) of reg128bits.
Now we will return false if any of the upper bits can't be described.

This change is of course needed, considering that we want to handle things like

DBG_VALUE $reg128bits, $noreg, !7, !DIExpression(DW_OP_constu 96, DW_OP_shr, DW_OP_LLVM_fragment, 0, 32)

when the most significant bits of reg128bits are of interest. But maybe we want to re-introduce the MaxSize in the future, but being more careful when to restrict the MaxSize.

One simple thing would be to set MaxSize when the DIExpression only contains a DW_OP_LLVM_fragment (i.e. we could still use MaxSize when HasComplexExpression is false).
In the long term (as a FIXME), we could analyse the DIExpression further to find out how many of the least significant bits in the register that are "demanded".

[aprantl]

In D73652#1849033, @bjope wrote:

Thanks for the NFC split!
I was a little bit confused at first (about "Diff 241236") but it just seems like you have uploaded the reverse diff (so the new code is to the left and the old code is to the right when looking at the diff in Phabricator).

Nice, so I screwed it up twice m-(

I tried to get the previous diff by doing git diff -U999 HEAD~1..HEAD~2 and got the order wrong.

One worry is that for input like this

DBG_VALUE $reg128bits, $noreg, !7, !DIExpression(DW_OP_LLVM_fragment, 0, 32)

where reg128bits would be the composition of four 32-bit registers on some imaginary target,
then we used to set MaxSize=32 and then we could return true from addMachineReg after finding the first subregister (covering the least significant bits) of reg128bits.
Now we will return false if any of the upper bits can't be described.

Agreed. Is that a hypothetical, or should we address this because such targets might actually exist?

This change is of course needed, considering that we want to handle things like

DBG_VALUE $reg128bits, $noreg, !7, !DIExpression(DW_OP_constu 96, DW_OP_shr, DW_OP_LLVM_fragment, 0, 32)

when the most significant bits of reg128bits are of interest. But maybe we want to re-introduce the MaxSize in the future, but being more careful when to restrict the MaxSize.

Right. It would only help in that exact edge case, but not with the complex expressions that nowadays fall out of salvageDebugInfo().

One simple thing would be to set MaxSize when the DIExpression only contains a DW_OP_LLVM_fragment (i.e. we could still use MaxSize when HasComplexExpression is false).
In the long term (as a FIXME), we could analyse the DIExpression further to find out how many of the least significant bits in the register that are "demanded".

[aprantl]

Add comment as suggested.

[bjope]

So if I understand correctly this patch is doing lots of things:

1. Changing some types from unsigned to uint16_t. NFC
2. Some renaming. NFC
3. Introducing Register::Kind and refactoring how Register comments are handled. NFC:ish (considering that the "no DWARF register encoding" kind seems to be removed)
4. Inlining maskSubRegister. NFC (?)
5. Clean up DwarfRegs using ClearDwarfRegs. NFC
6. A change at line 340 to use Register::SubRegSize (called Register::SubRegisterSizeInBits after the patch) instead of DwarfExpression::SubRegisterSizeInBits. NFC (?)
7. Making sure we discard debug info when the full register can't be encoded, instead of padding with unknown pieces. A functional change IMO.
8. Moving some logic related to fragments from addMachineRegExpression to addMachineReg. I guess this is the "deferring" which should be the main act in this patch.

My thoughts about those, separated from each other:

(1), (2), (3), (4) and (5) seems like good things to do regardless of the rest.

(6) As mentioned inline, this change seems to be NFC, but I can't see any purpose with changing it.

(7) Is probably ok. As discussed in earlier comments this could hypotetically make things worse for some targets when only some bits from the register really need to be described (given the size of the fragment to describe). Could be interesting to have this change as a separate commit if we move forward with it, to be able to bisect any potential problems to the specific commit. It would also allow me to test that commit separately, to see if I get different results for a given target when adding that commit.

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

262

Now when we got the offset included in the Register struct we could assert that subregister pieces are ordered from least significant to most significant in the DwardRegs vector:

assert(BitsEmitted == Reg.SubRegisterOffsetInBits)

I think that as a follow up we should consider if the iteration order should be reversed for big endian targets (at least we need to do it for our out-of-tree target, since in DWARF the pieces should be in increasing memory order). That would ofcourse make such an assertion to fail, as well as complicating the SubRegBits calculcation a little bit.

316

nit: "full register" -> "full/super register"

340

It took me awhile to understand this change.

I guess SubRegisterSizeInBits is equal to Reg.SubRegisterSizeInBits all the time here (given the assert above that we only get here with Reg.Kind!=Register::SubReg).
Or am I missing some corner case for which this change really is needed?

Since it is related to SubRegisterSizeInBits and that the piece stuff won't be added until later if SubRegisterSizeInBits!=0, maybe it is better to keep the old notation here? Reg.SubRegisterSizeInBits is mainly only used for Register::SubReg afaict (except this usage).

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

147

I think these two members could be prefixed with "SuperRegisterPiece..." instead of "SubRegister...", now when they are set by setSuperRegisterPiece. This also avoids potential confusion with the renaming of the members in the Register struct.

275

Not really related to this patch, but LocationFlags(Unknown) looks weird since the Unknown is part of the enum listing kinds, but not the enum listing flags. Using LocationFlags(0) is perhaps better if it needs to be initialized. To avoid the weird dependency to the values used in the enumeration with location kinds.

[bjope]

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

262

You may skip this part. After some testing of this patch, trying to adapt our downstream fixes, I'd prefer to get rid of the BitsEmitted (see comment below about using Reg.SubRegisterOffsetInBits instead to determine the start position of each subreg.

269

Can we change this part to

case Register::SubReg: {
  // If the last subregister(s) extend beyond the size of the
  // fragment, truncate the DW_OP_piece accordingly. This is safe
  // because we have a simple expression.
  unsigned SubRegBits = Reg.SubRegisterSizeInBits;
  unsigned SubRegOffset = Reg.SubRegisterOffsetInBits;
  if (SubRegBits == 0 || SubRegOffset >= FragmentBits)
    break;
  if (SubRegOffset + SubRegBits > FragmentBits &&
      FragmentBits > SubRegOffset)
    SubRegBits = FragmentBits - SubRegOffset;
  if (Reg.DwarfRegNo >= 0)
    addReg(Reg.DwarfRegNo, Reg.getComment());
  if (SubRegBits < FragmentBits)
    addOpPiece(SubRegBits);
  break;
}

This way we do not need BitsEmitted, and the code works also if case the DwarfRegs vector is reversed (which is something I think is needed for big endian targets).

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h

 /// buffer, which can later on be committed to the main output. The size of the
 /// temporary buffer is queryable, allowing for the size of the data to be
 /// emitted before the data is committed.
 class DwarfExpression {
 protected:
   /// Holds information about all subregisters comprising a register location.
   struct Register {
     int DwarfRegNo;
-    unsigned SubRegSize;
-    const char *Comment;
+    uint16_t SubRegisterSizeInBits;
+    uint16_t SubRegisterOffsetInBits;
+    enum : char { Full, SuperReg, SubReg } Kind;
 
     /// Create a full register, no extra DW_OP_piece operators necessary.
-    static Register createRegister(int RegNo, const char *Comment) {
-      return {RegNo, 0, Comment};
+    static Register create(int RegNo) {
+      return {RegNo, 0, 0, Full};
+    }
+
+    /// Stencil a subregister out of a superregister.
+    static Register createSuperReg(int RegNo, uint16_t SizeInBits,
+                                   uint16_t OffsetInBits) {
+      return {RegNo, SizeInBits, OffsetInBits, SuperReg};
     }
 
     /// Create a subregister that needs a DW_OP_piece operator with SizeInBits.
-    static Register createSubRegister(int RegNo, unsigned SizeInBits,
-                                      const char *Comment) {
-      return {RegNo, SizeInBits, Comment};
+    static Register createSubReg(int RegNo, uint16_t SizeInBits,
+                                 uint16_t OffsetInBits) {
+      return {RegNo, SizeInBits, OffsetInBits, SubReg};
     }
 
-    bool isSubRegister() const { return SubRegSize; }
+    /// Return the assembler comment for this register.
+    const char *getComment() const;
   };
 
   /// Whether we are currently emitting an entry value operation.
   bool IsEmittingEntryValue = false;
 
   DwarfCompileUnit &CU;
 
   /// The register location, if any.
   SmallVector<Register, 2> DwarfRegs;
 
   /// Current Fragment Offset in Bits.
   uint64_t OffsetInBits = 0;
 
   /// Sometimes we need to add a DW_OP_bit_piece to describe a subregister.
-  unsigned SubRegisterSizeInBits : 16;
-  unsigned SubRegisterOffsetInBits : 16;
+  uint16_t SubRegisterSizeInBits = 0;
+  uint16_t SubRegisterOffsetInBits = 0;
 
   /// The kind of location description being produced.
   enum { Unknown = 0, Register, Memory, Implicit };
 
   /// The flags of location description being produced.
   enum { EntryValue = 1, CallSiteParamValue };
 
   unsigned LocationKind : 3;
public:
   bool isEntryValue() const { return LocationFlags & EntryValue; }
 
   bool isParameterValue() { return LocationFlags & CallSiteParamValue; }
 
   Optional<uint8_t> TagOffset;
 
 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) {
-    assert(SizeInBits < 65536 && OffsetInBits < 65536);
+  /// to represent a register as a piece of a superregister.
+  void setSuperRegisterPiece(uint16_t SizeInBits, uint16_t OffsetInBits) {
     SubRegisterSizeInBits = SizeInBits;
     SubRegisterOffsetInBits = OffsetInBits;
   }
 
-  /// Add masking operations to stencil out a subregister.
-  void maskSubRegister();
-
   /// Output a dwarf operand and an optional assembler comment.
   virtual void emitOp(uint8_t Op, const char *Comment = nullptr) = 0;
 
   /// Emit a raw signed value.
   virtual void emitSigned(int64_t Value) = 0;
 
   /// Emit a raw unsigned value.
   virtual void emitUnsigned(uint64_t Value) = 0;
protected:
   virtual bool isFrameRegister(const TargetRegisterInfo &TRI,
                                unsigned MachineReg) = 0;
 
   /// Emit a DW_OP_reg operation. Note that this is only legal inside a DWARF
   /// register location description.
   void addReg(int DwarfReg, const char *Comment = nullptr);
 
   /// Emit a DW_OP_breg operation.
-  void addBReg(int DwarfReg, int Offset);
+  void addBReg(int DwarfReg, int Offset, const char *Comment = nullptr);
 
   /// Emit DW_OP_fbreg <Offset>.
   void addFBReg(int Offset);
 
   /// Emit a partial DWARF register operation.
   ///
   /// \param MachineReg           The register number.
-  /// \param MaxSize              If the register must be composed from
-  ///                             sub-registers this is an upper bound
-  ///                             for how many bits the emitted DW_OP_piece
-  ///                             may cover.
   ///
   /// If size and offset is zero an operation for the entire register is
   /// emitted: Some targets do not provide a DWARF register number for every
   /// register.  If this is the case, this function will attempt to emit a DWARF
   /// register by emitting a fragment of a super-register or by piecing together
   /// multiple subregisters that alias the register.
   ///
   /// \return false if no DWARF register exists for MachineReg.
-  bool addMachineReg(const TargetRegisterInfo &TRI, unsigned MachineReg,
-                     unsigned MaxSize = ~1U);
+  bool addMachineReg(const TargetRegisterInfo &TRI, unsigned MachineReg);
 
   /// Emit a DW_OP_piece or DW_OP_bit_piece operation for a variable fragment.
   /// \param OffsetInBits    This is an optional offset into the location that
   /// is at the top of the DWARF stack.
   void addOpPiece(unsigned SizeInBits, unsigned OffsetInBits = 0);
 
   /// Emit a shift-right dwarf operation.
   void addShr(unsigned ShiftBy);
protected:
   /// Finalize an entry value by emitting its size operand, and committing the
   /// DWARF block which has been emitted to the temporary buffer.
   void finalizeEntryValue();
 
   ~DwarfExpression() = default;
 
 public:
   DwarfExpression(unsigned DwarfVersion, DwarfCompileUnit &CU)
-      : CU(CU), SubRegisterSizeInBits(0), SubRegisterOffsetInBits(0),
-        LocationKind(Unknown), LocationFlags(Unknown),
+      : CU(CU), LocationKind(Unknown), LocationFlags(Unknown),
         DwarfVersion(DwarfVersion) {}
 
   /// This needs to be called last to commit any pending changes.
   void finalize();
 
   /// Emit a signed constant.
   void addSignedConstant(int64_t Value);
 

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

 //===- llvm/CodeGen/DwarfExpression.cpp - Dwarf Debug Framework -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains support for writing dwarf debug info into asm files.
 //
 //===----------------------------------------------------------------------===//
 
 #include "DwarfExpression.h"
 #include "DwarfCompileUnit.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallBitVector.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/CodeGen/Register.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
 #include <cassert>
void DwarfExpression::addReg(int DwarfReg, const char *Comment) {
   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) {
+void DwarfExpression::addBReg(int DwarfReg, int Offset, const char *Comment) {
   assert(DwarfReg >= 0 && "invalid negative dwarf register number");
   assert(!isRegisterLocation() && "location description already locked down");
   if (DwarfReg < 32) {
     emitOp(dwarf::DW_OP_breg0 + DwarfReg);
   } else {
-    emitOp(dwarf::DW_OP_bregx);
+    emitOp(dwarf::DW_OP_bregx, Comment);
     emitUnsigned(DwarfReg);
   }
   emitSigned(Offset);
 }
 
 void DwarfExpression::addFBReg(int Offset) {
   emitOp(dwarf::DW_OP_fbreg);
   emitSigned(Offset);

 }
 
 void DwarfExpression::addAnd(unsigned Mask) {
   emitConstu(Mask);
   emitOp(dwarf::DW_OP_and);
 }
 
 bool DwarfExpression::addMachineReg(const TargetRegisterInfo &TRI,
-                                    unsigned MachineReg, unsigned MaxSize) {
+                                    unsigned MachineReg) {
   if (!llvm::Register::isPhysicalRegister(MachineReg)) {
     if (isFrameRegister(TRI, MachineReg)) {
-      DwarfRegs.push_back(Register::createRegister(-1, nullptr));
+      DwarfRegs.push_back(Register::create(-1));
       return true;
     }
     return false;
   }
 
   int Reg = TRI.getDwarfRegNum(MachineReg, false);
 
   // If this is a valid register number, emit it.
   if (Reg >= 0) {
-    DwarfRegs.push_back(Register::createRegister(Reg, nullptr));
+    DwarfRegs.push_back(Register::create(Reg));
     return true;
   }
 
   // Walk up the super-register chain until we find a valid number.
   // For example, EAX on x86_64 is a 32-bit fragment of RAX with offset 0.
   for (MCSuperRegIterator SR(MachineReg, &TRI); SR.isValid(); ++SR) {
     Reg = TRI.getDwarfRegNum(*SR, false);
     if (Reg >= 0) {
       unsigned Idx = TRI.getSubRegIndex(*SR, MachineReg);
       unsigned Size = TRI.getSubRegIdxSize(Idx);
       unsigned RegOffset = TRI.getSubRegIdxOffset(Idx);
-      DwarfRegs.push_back(Register::createRegister(Reg, "super-register"));
       // Use a DW_OP_bit_piece to describe the sub-register.
-      setSubRegisterPiece(Size, RegOffset);
+      DwarfRegs.push_back(Register::createSuperReg(Reg, Size, RegOffset));
+      setSuperRegisterPiece(Size, RegOffset);
       return true;
     }
   }
 
   // Otherwise, attempt to find a covering set of sub-register numbers.
   // For example, Q0 on ARM is a composition of D0+D1.
   unsigned CurPos = 0;
   // The size of the register in bits.
for (MCSubRegIterator SR(MachineReg, &TRI); SR.isValid(); ++SR) {
 
     // Used to build the intersection between the bits we already
     // emitted and the bits covered by this subregister.
     SmallBitVector CurSubReg(RegSize, false);
     CurSubReg.set(Offset, Offset + Size);
 
     // If this sub-register has a DWARF number and we haven't covered
     // its range, and its range covers the value, emit a DWARF piece for it.
-    if (Offset < MaxSize && CurSubReg.test(Coverage)) {
-      // Emit a piece for any gap in the coverage.
+    if (CurSubReg.test(Coverage)) {
+      // Give up if there is a gap in the coverage.  Note that if the
+      // register is immediately followed by a DW_OP_piece or some
+      // other operation that truncates the undescribed bits, we could
+      // still salvage the expression here.
       if (Offset > CurPos)
-        DwarfRegs.push_back(Register::createSubRegister(
-            -1, Offset - CurPos, "no DWARF register encoding"));
-      if (Offset == 0 && Size >= MaxSize)
-        DwarfRegs.push_back(Register::createRegister(Reg, "sub-register"));
-      else
-        DwarfRegs.push_back(Register::createSubRegister(
-            Reg, std::min<unsigned>(Size, MaxSize - Offset), "sub-register"));
+        return false;
+      DwarfRegs.push_back(Register::createSubReg(Reg, Size, Offset));
     }
     // Mark it as emitted.
     Coverage.set(Offset, Offset + Size);
     CurPos = Offset + Size;
   }
   // Failed to find any DWARF encoding.
   if (CurPos == 0)
     return false;
   // Found a partial or complete DWARF encoding.
   if (CurPos < RegSize)
-    DwarfRegs.push_back(Register::createSubRegister(
-        -1, RegSize - CurPos, "no DWARF register encoding"));
+    return false;
   return true;
 }
 
+const char *DwarfExpression::Register::getComment() const {
+  switch(Kind) {
+  case Register::Full:     return nullptr;
+  case Register::SuperReg: return "super-register";
+  case Register::SubReg:   return "sub-register";
+  }
+}
+
 void DwarfExpression::addStackValue() {
   if (DwarfVersion >= 4)
     emitOp(dwarf::DW_OP_stack_value);
 }
 
 void DwarfExpression::addSignedConstant(int64_t Value) {
   assert(isImplicitLocation() || isUnknownLocation());
   LocationKind = Implicit;
while (Offset < Size) {
     Offset += 64;
   }
 }
 
 bool DwarfExpression::addMachineRegExpression(const TargetRegisterInfo &TRI,
                                               DIExpressionCursor &ExprCursor,
                                               unsigned MachineReg,
                                               unsigned FragmentOffsetInBits) {
-  auto Fragment = ExprCursor.getFragmentInfo();
-  if (!addMachineReg(TRI, MachineReg, Fragment ? Fragment->SizeInBits : ~1U)) {
+  auto ClearDwarfRegs = make_scope_exit([this] { DwarfRegs.clear(); });
+
+  if (!addMachineReg(TRI, MachineReg)) {
     LocationKind = Unknown;
     return false;
   }
 
   bool HasComplexExpression = false;
   auto Op = ExprCursor.peek();
   if (Op && Op->getOp() != dwarf::DW_OP_LLVM_fragment)
     HasComplexExpression = true;
 
-  // If the register can only be described by a complex expression (i.e.,
-  // multiple subregisters) it doesn't safely compose with another complex
-  // expression. For example, it is not possible to apply a DW_OP_deref
-  // operation to multiple DW_OP_pieces.
-  if (HasComplexExpression && DwarfRegs.size() > 1) {
-    DwarfRegs.clear();
-    LocationKind = Unknown;
+  // If the register itself can only be described by combining
+  // multiple subregisters, we cannot use DW_OP_piece, since it
+  // doesn't safely compose with another complex expression, Since it
+  // is not possible to apply any DWARF operation to the combined
+  // DW_OP_pieces.
+  if (HasComplexExpression && DwarfRegs.size() > 1)
     return false;
-  }
 
   // Handle simple register locations. If we are supposed to emit
   // a call site parameter expression and if that expression is just a register
   // location, emit it with addBReg and offset 0, because we should emit a DWARF
   // expression representing a value, rather than a location.
   if (!isMemoryLocation() && !HasComplexExpression &&
       (!isParameterValue() || isEntryValue())) {
+    unsigned BitsEmitted = 0;
+    auto Fragment = ExprCursor.getFragmentInfo();
+    unsigned FragmentBits =
+        Fragment ? Fragment->SizeInBits : std::numeric_limits<unsigned>::max();
     for (auto &Reg : DwarfRegs) {
+      switch (Reg.Kind) {
+      case Register::Full:
+      case Register::SuperReg:
         if (Reg.DwarfRegNo >= 0)
-        addReg(Reg.DwarfRegNo, Reg.Comment);
-      addOpPiece(Reg.SubRegSize);
+          addReg(Reg.DwarfRegNo, Reg.getComment());
+        break;
+      case Register::SubReg: {
+        // If the last subregister(s) extend beyond the size of the
+        // fragment, truncate the DW_OP_piece accordingly. This is safe
+        // because we have a simple expression.
+        unsigned SubRegBits = Reg.SubRegisterSizeInBits;
+        if (BitsEmitted + SubRegBits > FragmentBits)
+          SubRegBits -= (BitsEmitted + SubRegBits) - FragmentBits;
+        BitsEmitted += SubRegBits;
+        if (SubRegBits == 0)
+          break;
+        if (Reg.DwarfRegNo >= 0)
+          addReg(Reg.DwarfRegNo, Reg.getComment());
+        if (SubRegBits < FragmentBits)
+          addOpPiece(SubRegBits);
+        break;
+      }
+      }
     }
       
     if (isEntryValue())
       finalizeEntryValue();
 
     if (isEntryValue() && !isParameterValue() && DwarfVersion >= 4)
       emitOp(dwarf::DW_OP_stack_value);
 
-    DwarfRegs.clear();
     return true;
   }
 
   // Don't emit locations that cannot be expressed without DW_OP_stack_value.
   if (DwarfVersion < 4)
     if (any_of(ExprCursor, [](DIExpression::ExprOperand Op) -> bool {
           return Op.getOp() == dwarf::DW_OP_stack_value;
         })) {
-      DwarfRegs.clear();
       LocationKind = Unknown;
       return false;
     }
 
-  assert(DwarfRegs.size() == 1);
-  auto Reg = DwarfRegs[0];
+  if (DwarfRegs.size() != 1) {
+    // While this could happen legitimately, it is more likely to
+    // indicate a bug.
+    assert(isMemoryLocation() && "pointer spawns multiple registers");
+    return false;
+  }
+
+  auto Reg = DwarfRegs.front();
   bool FBReg = isFrameRegister(TRI, MachineReg);
   int SignedOffset = 0;
-  assert(!Reg.isSubRegister() && "full register expected");
+  assert(Reg.Kind != Register::SubReg && "full register expected");
 
   // Pattern-match combinations for which more efficient representations exist.
   // [Reg, DW_OP_plus_uconst, Offset] --> [DW_OP_breg, Offset].
   if (Op && (Op->getOp() == dwarf::DW_OP_plus_uconst)) {
     uint64_t Offset = Op->getArg(0);
     uint64_t IntMax = static_cast<uint64_t>(std::numeric_limits<int>::max());
     if (Offset <= IntMax) {
       SignedOffset = Offset;
       ExprCursor.take();
     }
   }
 
   // [Reg, DW_OP_constu, Offset, DW_OP_plus]  --> [DW_OP_breg, Offset]
   // [Reg, DW_OP_constu, Offset, DW_OP_minus] --> [DW_OP_breg,-Offset]
   // If Reg is a subregister we need to mask it out before subtracting.
   if (Op && Op->getOp() == dwarf::DW_OP_constu) {
     uint64_t Offset = Op->getArg(0);
     uint64_t IntMax = static_cast<uint64_t>(std::numeric_limits<int>::max());
     auto N = ExprCursor.peekNext();
     if (N && N->getOp() == dwarf::DW_OP_plus && Offset <= IntMax) {
       SignedOffset = Offset;
       ExprCursor.consume(2);
     } else if (N && N->getOp() == dwarf::DW_OP_minus &&
-               !SubRegisterSizeInBits && Offset <= IntMax + 1) {
+               !Reg.SubRegisterSizeInBits && Offset <= IntMax + 1) {
       SignedOffset = -static_cast<int64_t>(Offset);
       ExprCursor.consume(2);
     }
   }
 
   if (FBReg)
     addFBReg(SignedOffset);
   else
     addBReg(Reg.DwarfRegNo, SignedOffset);
-  DwarfRegs.clear();
   return true;
 }
 
 void DwarfExpression::beginEntryValueExpression(
     DIExpressionCursor &ExprCursor) {
   auto Op = ExprCursor.take();
   (void)Op;
   assert(Op && Op->getOp() == dwarf::DW_OP_LLVM_entry_value);
default:
       return false;
     }
   }
   return true;
 }
 
 void DwarfExpression::addExpression(DIExpressionCursor &&ExprCursor,
                                     unsigned FragmentOffsetInBits) {
+  auto N = ExprCursor.peek();
+
   // If we need to mask out a subregister, do it now, unless the next
   // operation would emit an OpPiece anyway.
-  auto N = ExprCursor.peek();
-  if (SubRegisterSizeInBits && N && (N->getOp() != dwarf::DW_OP_LLVM_fragment))
-    maskSubRegister();
+  if (SubRegisterSizeInBits && N && (N->getOp() != dwarf::DW_OP_LLVM_fragment)) {
+    if (SubRegisterOffsetInBits > 0)
+      addShr(SubRegisterOffsetInBits);
+    uint64_t Mask = (1ULL << (uint64_t)SubRegisterSizeInBits) - 1ULL;
+    addAnd(Mask);
+  }
 
   Optional<DIExpression::ExprOperand> PrevConvertOp = None;
 
   while (ExprCursor) {
     auto Op = ExprCursor.take();
     uint64_t OpNum = Op->getOp();
 
     if (OpNum >= dwarf::DW_OP_reg0 && OpNum <= dwarf::DW_OP_reg31) {
case dwarf::DW_OP_LLVM_fragment: {
         SizeInBits = std::min<unsigned>(SizeInBits, SubRegisterSizeInBits);
 
       // Emit a DW_OP_stack_value for implicit location descriptions.
       if (isImplicitLocation())
         addStackValue();
 
       // Emit the DW_OP_piece.
       addOpPiece(SizeInBits, SubRegisterOffsetInBits);
-      setSubRegisterPiece(0, 0);
+      setSuperRegisterPiece(0, 0);
       // Reset the location description kind.
       LocationKind = Unknown;
       return;
     }
     case dwarf::DW_OP_plus_uconst:
       assert(!isRegisterLocation());
       emitOp(dwarf::DW_OP_plus_uconst);
       emitUnsigned(Op->getArg(0));
while (ExprCursor) {
     }
   }
 
   if (isImplicitLocation() && !isParameterValue())
     // 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)
-    addShr(SubRegisterOffsetInBits);
-  uint64_t Mask = (1ULL << (uint64_t)SubRegisterSizeInBits) - 1ULL;
-  addAnd(Mask);
-}
-
 void DwarfExpression::finalize() {
   assert(DwarfRegs.size() == 0 && "dwarf registers not emitted");
   // Emit any outstanding DW_OP_piece operations to mask out subregisters.
   if (SubRegisterSizeInBits == 0)
     return;
   // Don't emit a DW_OP_piece for a subregister at offset 0.
   if (SubRegisterOffsetInBits == 0)
     return;