[PATCH] D67556: [ARM][AArch64][DebugInfo] Improve call site instruction interpretation

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: include/llvm/CodeGen/TargetInstrInfo.h:888
+  /// If the specific machine instruction is an instruction that adds an
+  /// immediate value to its first operand and stores it in the first, return
+  /// true along with @Source machine operand to which @Offset has been
----------------
NikolaPrica wrote:
> dstenb wrote:
> > NikolaPrica wrote:
> > > dstenb wrote:
> > > > dstenb wrote:
> > > > > I wonder if the hook should allow the source and destination to be different, as we then for example could describe cases like this:
> > > > > 
> > > > > ```
> > > > > $reg0 = add $frame-ptr, -13
> > > > > ```
> > > > > 
> > > > > If so, would it then make sense to move the LEA part of X86's `describeLoadedValue()` hook into this hook instead?
> > > > If so, should we perhaps also consider generalizing the hook so that it has a Destination out-parameter, e.g. same as `isCopyInstr()`? That could probably be helpful if we make the `describeLoadedValue()` hook aware of which register it should describe, as we discussed in D67225.
> > > > I wonder if the hook should allow the source and destination to be different
> > > 
> > > 
> > > In fact we should only relay on situations were source and destination operands are different. Such restriction should be used at general part of `describeLoadedValue()`. There is no use of describing situatios like
> > > 
> > > $reg0 = add $reg0, 4
> > > 
> > > This case would require recursive description of $reg0. Describing such instruction is a different story.
> > > 
> > > > If so, would it then make sense to move the LEA part of X86's describeLoadedValue() hook into this hook instead?
> > > 
> > > The LEA instruction is more complex than add immidiate instruction. It could be observed as add immidiate for one case but it could also have addition of multiple source registers with some multiplication operations. IMHO it would be better to keep this API function clean in sense of recognizing only clear add immidiate instruction for purpose of further usage.
> > > 
> > > > If so, should we perhaps also consider generalizing the hook so that it has a Destination out-parameter, e.g. same as isCopyInstr()? That could probably be helpful if we make the describeLoadedValue() hook aware of which register it should describe, as we discussed in D67225.
> > > 
> > > In the first version of this function I've added a destination operand but I've removed it since there was no current use of it. But for further flexibilty I will add it.
> > > 
> > > In fact we should only relay on situations were source and destination operands are different. Such restriction should be used at general part of describeLoadedValue(). There is no use of describing situatios like
> > >
> > > $reg0 = add $reg0, 4
> > 
> > In previous revisions of the downstream target we develop for we had to resort to:
> > 
> > ```
> > $reg0 = mov $frame-ptr
> > $reg0 = add $reg0, $offset
> > ```
> > 
> > instead of loading the frame pointer with an offset in one instruction. Perhaps there is some upstream target that requires the same?
> > 
> > > This case would require recursive description of $reg0. Describing such instruction is a different story.
> > 
> > Is that due to the issue with expressions in collectCallSiteParameters() which we discussed earlier in this patch?
> > 
> > > The LEA instruction is more complex than add immidiate instruction. It could be observed as add immidiate for one case but it could also have addition of multiple source registers with some multiplication operations. IMHO it would be better to keep this API function clean in sense of recognizing only clear add immidiate instruction for purpose of further usage.
> > 
> > Okay, that sounds fair. Moving some parts to the LEA implementation to this hook, and keeping the rest in `describeLoadedValue()` would probably not be ideal.
> > 
> > > In the first version of this function I've added a destination operand but I've removed it since there was no current use of it. But for further flexibilty I will add it.
> > 
> > Okay, thanks!
> > Is that due to the issue with expressions in collectCallSiteParameters() which we discussed earlier in this patch?
> 
> Yes. You are right. Such cases should be handled the way we discussed there. But until such support is provided such instructions ($reg0 = add $reg0, 4) should be omitted. I will emphasize that as TODO comment.
> 
> [...] adds an immediate value to its first operand and stores it in the first, [...]

This part of the comment needs to be updated to reflect that the destination and source can be any operands.


================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:5335
+  // TODO: Third operand can be global address (usually some string). Since
+  //       strings can be relocated we cannot calculate theirs offsets for
+  //       now.
----------------
nit: //theirs -> their//


================
Comment at: test/DebugInfo/MIR/AArch64/dbgcall-site-interpretation.mir:2
+# RUN: llc -mtriple aarch64-linux-gnu -debug-entry-values -start-after=machineverifier -filetype=obj %s -o -| llvm-dwarfdump -| FileCheck %s
+# CHECK: DW_TAG_GNU_call_site
+# CHECK-NEXT: DW_AT_abstract_origin {{.*}} "func2"
----------------
I'm sorry for such a late comment about this, but if you still have the C reproducers readily available I think it would be helpful to add them to the tests (assuming that the IR or MIR haven't been substantially modified).


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

https://reviews.llvm.org/D67556