[PATCH] D73652: Defer composing subregisters with DW_OP_piece

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:262
+        Fragment ? Fragment->SizeInBits : std::numeric_limits<unsigned>::max();
     for (auto &Reg : DwarfRegs) {
+      switch (Reg.Kind) {
----------------
bjope wrote:
> 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.
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.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:269
+        break;
+      case Register::SubReg: {
+        // If the last subregister(s) extend beyond the size of the
----------------
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73652/new/

https://reviews.llvm.org/D73652