[PATCH] D73652: Defer composing subregisters with DW_OP_piece

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

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.



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


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:316
   int SignedOffset = 0;
-  assert(!Reg.isSubRegister() && "full register expected");
+  assert(Reg.Kind != Register::SubReg && "full register expected");
 
----------------
nit: "full register" -> "full/super register"


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:340
     } else if (N && N->getOp() == dwarf::DW_OP_minus &&
-               !SubRegisterSizeInBits && Offset <= IntMax + 1) {
+               !Reg.SubRegisterSizeInBits && Offset <= IntMax + 1) {
       SignedOffset = -static_cast<int64_t>(Offset);
----------------
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).


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:147
   /// 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;
----------------
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.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.h:275
   DwarfExpression(unsigned DwarfVersion, DwarfCompileUnit &CU)
-      : CU(CU), SubRegisterSizeInBits(0), SubRegisterOffsetInBits(0),
-        LocationKind(Unknown), LocationFlags(Unknown),
+      : CU(CU), LocationKind(Unknown), LocationFlags(Unknown),
         DwarfVersion(DwarfVersion) {}
----------------
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.


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

https://reviews.llvm.org/D73652