[PATCH] D90020: [AArch64][SVE] Emit DWARF location expression for SVE stack objects.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen marked an inline comment as done.
sdesmalen added a comment.
Herald added a subscriber: NickHung.

In D90020#2436998 <https://reviews.llvm.org/D90020#2436998>, @jmorse wrote:

> 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?



================
Comment at: llvm/lib/IR/DebugInfoMetadata.cpp:1143
+      Ops.push_back(-ScaledOffset);
+      Ops.append({dwarf::DW_OP_bregx, *ScaleReg, 0ULL});
+      Ops.push_back(dwarf::DW_OP_mul);
----------------
StephenTozer wrote:
> sdesmalen wrote:
> > aprantl wrote:
> > > 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.
> > 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 [[ https://github.com/ARM-software/abi-aa/blob/master/aadwarf64/aadwarf64.rst | 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?
> 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.
Thank you for confirming @StephenTozer!


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

https://reviews.llvm.org/D90020