Page MenuHomePhabricator

Defer composing subregisters with DW_OP_piece
Needs ReviewPublic

Authored by aprantl on Jan 29 2020, 11:25 AM.

Details

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.

Diff Detail

Event Timeline

aprantl created this revision.Jan 29 2020, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 11:25 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl updated this revision to Diff 241235.Jan 29 2020, 11:30 AM

Factored out the NFC bits.

aprantl updated this revision to Diff 241236.Jan 29 2020, 11:31 AM

... and I updated the wrong review again.

bjope added a comment.Jan 30 2020, 3:06 AM

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".

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 updated this revision to Diff 241481.Jan 30 2020, 8:48 AM

Add comment as suggested.

bjope added a comment.Feb 2 2020, 12:06 PM

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 added inline comments.Feb 10 2020, 12:23 PM
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).