This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Emit DWARF location expression for SVE stack objects.
ClosedPublic

Authored by sdesmalen on Oct 23 2020, 3:03 AM.

Details

Summary

Extend PEI to emit a DWARF expression for StackOffsets that have
a fixed and scalable component. This means the expression that needs
to be added is either:

<base> + offset

or:

<base> + offset + scalable_offset * scalereg

where for SVE, the scale reg is the Vector Granule Dwarf register, which
encodes the number of 64bit 'granules' in an SVE vector and which
the debugger can evaluate at runtime.

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 23 2020, 3:03 AM

Adding @ostannard who might have looked at DWARF info before.

Gentle ping.

(Or perhaps someone has a suggestion for a reviewer?)

aprantl added inline comments.Nov 17 2020, 6:09 PM
llvm/lib/IR/DebugInfoMetadata.cpp
1143 ↗(On Diff #300205)

We currently don't allow hardcoded registers inside of a DIExpression, The clean solution for this would be to rebase this on the patch that adds support for DW_OP_LLVM_argX / DBG_VALUE_LIST.

sdesmalen added inline comments.Nov 23 2020, 1:52 PM
llvm/lib/IR/DebugInfoMetadata.cpp
1143 ↗(On Diff #300205)

Hi @aprantl, thanks for having a look at this patch.

I looked at the proposal for DW_OP_LLVM_argX/DBG_VALUE_LIST and it seems that the scope of that work is a lot wider than what's needed for this purpose and I don't immediately see how I would represent these expressions using that mechanism, specifically because ScaleReg itself has no value in IR/MIR.

Just to clarify the use-case for AArch64 SVE: ScaleReg is always the same register, namely register 46 which is the DWARF register that contains the value for VG, the number of 64-bit "vector granules " in a scalable vector. During the program VG is constant. A runtime value is needed, because the length of the vector isn't known at compile-time. Hence the reason that DWARF for the Arm® 64-bit Architecture (AArch64) specifies the VG register to contain this value, so it can be used in forming DWARF expressions, e.g. to say that an offset (or number of elements when describing a vector type) is scaled by VG.

I can see why it is not customary or considered bad practice to hard-code registers directly in a DIExpression, but in this case I don't think this is really this a problem, given that in this context the register should actually be a hardcoded register.

Given this somewhat limited use-case, I'm not sure if the concern here is with the interface (i.e. we don't want to expose an explicit ScaleReg in this interface, as that opens the interface up to wrong uses of it), or whether the problem really is with the data that the DIExpression contains. i.e. Is there anything fundamental that prevents a DIExpression from containing a hard-coded register?

If not, I could modify the patch to have the target return a self-built DIExpression for the given offset, that can be prepended in PEI. If that would be fine, then I would much prefer that route over rebasing on what seems like a significant functional refactoring effort, which may take a while before it all lands.

What do you think?

sdesmalen updated this revision to Diff 308691.Dec 1 2020, 9:57 AM

Refactored the patch so that DIExpression no longer exposes a possibility to hard-code a register. Instead this patch adds TargetRegisterInfo::prependOffsetExpression, which can be overridden by the target to generate an expression for a StackOffset if part of that offset is scalable (and thus needs a target-specific expression to describe the offset). For SVE it generates the expression manually in the form of a SmallVector<uint64_t> and prepends it to the given DIExpression. The default implementation in TargetRegisterInfo will use DIExpression::prepend() with a byte-sized offset.

I think the approach here looks good to me, but maybe give others a chance to review first?

llvm/lib/Target/AArch64/AArch64RegisterInfo.cpp
618

nit: Additional whitespace

llvm/test/CodeGen/AArch64/debug-info-sve-dbg-value.mir
16

I'm not very familiar with the DWARF dump so I apologise if I misunderstood something, but is this only testing either pure scalable or pure fixed offsets? Looking at fillOffsetExpression I thought there might be offsets made up of byte-sized and vg-sized offsets, i.e. with two DW_OP_plus commands.

sdesmalen updated this revision to Diff 309880.Dec 7 2020, 4:44 AM

Removed whitespace.

sdesmalen marked an inline comment as done.Dec 7 2020, 4:45 AM
sdesmalen added inline comments.
llvm/test/CodeGen/AArch64/debug-info-sve-dbg-value.mir
16

This is testing a combination of both, i.e.

(DW_OP_breg31 WSP+16), 
((DW_OP_lit16, DW_OP_bregx VG+0), DW_OP_mul),
DW_OP_plus

<=> (SP + 16) + (16 * VG)

(Responding to inline comments) I think the main concern with hard-coding registers in DIExpressions at this stage is that LLVM isn't able to identify when that register is clobbered later on, and terminate the variable location. However, as you say:

Just to clarify the use-case for AArch64 SVE: ScaleReg is always the same register, namely register 46 which is the DWARF register that contains the value for VG, the number of 64-bit "vector granules " in a scalable vector. During the program VG is constant. A runtime value is needed, because the length of the vector isn't known at compile-time.

To confirm my understanding, 'VG' isn't a conventional "register", but instead it's a target-specific way to signal "whatever the vector scale factor is right now"? Given that it's a runtime constant, I think that resolves the concerns about being a hard-coded value, it doesn't need to be tracked in the same way that conventional registers are.

On the whole, LGTM, but I'd wait a bit to see if @aprantl is happy with the explanation. (Adding debug-info group too).

llvm/test/CodeGen/AArch64/debug-info-sve-dbg-value.mir
10–11

Could I suggest adding a larger DIExpression as the input to this check, for example an extra DW_OP_uconst, 8, DW_OP_plus just to ensure expression composition through your prependOffsetExpression implementation is covered in this test.

I'm assuming that the "*svint32_t" types act like aggregates in some way, and so can't have any other DWARF expression operators applied to them.

42

Could you remove the attributes for #1; for debug-info tests un-necessary attributes become an un-necessary burden.

sdesmalen updated this revision to Diff 309914.Dec 7 2020, 7:29 AM
sdesmalen marked an inline comment as done.
  • Extended DIExpression to better test prepend.
  • Removed unnecessary attributes
sdesmalen marked 2 inline comments as done.Dec 7 2020, 7:30 AM

Hi @jmorse, thanks for reviewing this patch!

(Responding to inline comments) I think the main concern with hard-coding registers in DIExpressions at this stage is that LLVM isn't able to identify when that register is clobbered later on, and terminate the variable location. However, as you say:

Just to clarify the use-case for AArch64 SVE: ScaleReg is always the same register, namely register 46 which is the DWARF register that contains the value for VG, the number of 64-bit "vector granules " in a scalable vector. During the program VG is constant. A runtime value is needed, because the length of the vector isn't known at compile-time.

To confirm my understanding, 'VG' isn't a conventional "register", but instead it's a target-specific way to signal "whatever the vector scale factor is right now"? Given that it's a runtime constant, I think that resolves the concerns about being a hard-coded value, it doesn't need to be tracked in the same way that conventional registers are.

Yes, I think that's a good summary. It is indeed a way to describe a runtime constant, rather than describing a register, since there is no physical register associated with it.

On the whole, LGTM, but I'd wait a bit to see if @aprantl is happy with the explanation. (Adding debug-info group too).

Thanks, will do!

llvm/test/CodeGen/AArch64/debug-info-sve-dbg-value.mir
10–11

Could I suggest adding a larger DIExpression as the input to this check, for example an extra DW_OP_uconst, 8, DW_OP_plus just to ensure expression composition through your prependOffsetExpression implementation is covered in this test.

Good suggestion, I've updated the test.

I'm assuming that the "*svint32_t" types act like aggregates in some way, and so can't have any other DWARF expression operators applied to them.

I think that's right, svint32_t are more or less opaque vector types. Do you have an example of expressions that would apply for regular (fixed-width) vectors?

jmorse added inline comments.Dec 7 2020, 8:02 AM
llvm/test/CodeGen/AArch64/debug-info-sve-dbg-value.mir
10–11

Do you have an example of expressions that would apply for regular (fixed-width) vectors?

Doing a quick search, I see a couple of examples:

llvm/test/DebugInfo/MIR/ARM/split-superreg-complex.mir
llvm/test/CodeGen/AMDGPU/debug-value.ll

that appear to apply non-trivial DIExpressions to fixed width vectors. Exactly how to interpret them is a bit beyond me alas, it may or may not be a concern with scalable vectors.

StephenTozer added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
1143 ↗(On Diff #300205)

As the author of the DBG_VALUE_LIST patch I agree that it'd be a bad idea to rebase onto that patch, since this patch doesn't strictly need it and that patch will probably take some time to land. I'm not sure if there's really a problem with including a hard-coded register in a DIExpression before final DWARF emission. If at some point we wish to add validation that registers never appear in DIExpressions, or if another implementation of scalable vector implementation that uses a different register is added, then I think the answer would be to create a new DIExpression operator specifically to refer to the contents of the scalable vector register; this could then be replaced by the hardcoded register during DWARF emission.

sdesmalen marked an inline comment as done.Dec 11 2020, 7:53 AM

On the whole, LGTM, but I'd wait a bit to see if @aprantl is happy with the explanation. (Adding debug-info group too).

@jmorse, would you be happy to formally accept the patch?

@aprantl based on the discussion, are you happy with the approach set out in this patch?

llvm/lib/IR/DebugInfoMetadata.cpp
1143 ↗(On Diff #300205)

Thank you for confirming @StephenTozer!

jmorse accepted this revision.Dec 14 2020, 6:24 AM

LGTM

This revision is now accepted and ready to land.Dec 14 2020, 6:24 AM

Hi everyone, thanks for all the review comments! Unless anyone has strong objections I'd like to land this patch on Thursday on Sander's behalf, since he is on holiday at the moment.

dstenb added a subscriber: dstenb.Dec 15 2020, 8:23 AM

I'm sorry for chiming in so late here! I have a comment about the prependOffsetExpression target hook.

llvm/lib/CodeGen/TargetRegisterInfo.cpp
538

I think I would be better to have explicit bool arguments for each of the flags that are contained here. If we keep the mask here, we could later on, when introducing a new flag, end up in situations where the new flag would be ignored by some target implementations, and that could be hard to detect.

Another solution could be to have the deref, stack value and entry value handling in this function, and then have it invoke a getOffsetOpcodes(Offset, Ops) target hook, or something along those lines, that produces the target specific expression for the offset.

At the risk of me overlooking some details for why it would be feasible or practical, I think I would prefer the latter interface.

dstenb added inline comments.Dec 15 2020, 8:25 AM
llvm/lib/CodeGen/TargetRegisterInfo.cpp
538

would not be feasible or practical

sdesmalen updated this revision to Diff 314360.Jan 4 2021, 4:54 AM
  • Made TargetRegisterInfo::prependOffsetExpression non-virtual and changed it to handle the Prepend flags.
  • Added virtual method TargetRegisterInfo::getOffsetOpcodes, which gets called by prependOffsetExpression.

I'm sorry for chiming in so late here! I have a comment about the prependOffsetExpression target hook.

Thanks for the suggestion, you made a good point that this interface was error prone when new flags are added in the future. I've updated the patch and added the virtual interface getOffsetOpcodes alongside prependOffsetExpression (made non-virtual and now handles the Deref, Stack/Entry Value). Let me know if this is how you imagined it.

sdesmalen marked 2 inline comments as done.Jan 4 2021, 4:55 AM
dstenb added a comment.Jan 5 2021, 2:00 AM

I'm sorry for chiming in so late here! I have a comment about the prependOffsetExpression target hook.

Thanks for the suggestion, you made a good point that this interface was error prone when new flags are added in the future. I've updated the patch and added the virtual interface getOffsetOpcodes alongside prependOffsetExpression (made non-virtual and now handles the Deref, Stack/Entry Value). Let me know if this is how you imagined it.

Yes, thanks!

Since the prependOffsetExpression function is outside DIExpression perhaps it is worth just adding an assert that catches unknown flags?

assert((PrependFlags & ~(DerefBefore | DerefAfter | StackValue | EntryValue)) == 0 && "Unknown prepend flag");
sdesmalen updated this revision to Diff 314569.Jan 5 2021, 4:51 AM

Added assert.

Since the prependOffsetExpression function is outside DIExpression perhaps it is worth just adding an assert that catches unknown flags?

assert((PrependFlags & ~(DerefBefore | DerefAfter | StackValue | EntryValue)) == 0 && "Unknown prepend flag");

Great suggestion, I've added the assert!