Page MenuHomePhabricator

[CSInfo][ISEL] Call site info generation support for Mips
ClosedPublic

Authored by ntesic on Apr 14 2020, 6:35 AM.

Details

Summary

Debug entry values functionality provides debug information about
call sites and function parameters values at the call entry spot.
Condition for generating this type of information is
compiling with -g option and optimization level higher than zero(-O0).

In ISEL phase, while lowering call instructions, collect info about registers
that forward arguments into following function frame.
We store such info into MachineFunction of the caller function.
This is used very late, when dumping DWARF info about call site parameters.

The call site info is visible at MIR level, as callSites attribute of MachineFunction.
Also, when using unmodified parameter value inside callee it could be
described as DW_OP_entry_value expression.
To deal with callSites attribute, we should pass -emit-call-site-info option to llc.

This patch enables functionality in clang frontend and adds call site info generation
support for MIPS targets(mips, mipsel, mips64, mips64el).

Tests:
-call-site-info-output.ll - existence of valid callSites attribute of MachineFunction, describing place of call instr and register forwarded parameters;
-dbg-call-site-info-reg-d-split.ll - omit from callSites, parameters forwarded through MIPS $D (temporary) register;
-dw_op_entry_value_32bit.ll, dw_op_entry_value_64bit.ll - confirm usage of call site info from callSites with DW_OP_entry_value (32 bit or 64 bit targets).

Diff Detail

Event Timeline

ntesic created this revision.Apr 14 2020, 6:35 AM
aprantl added inline comments.Apr 15 2020, 4:02 PM
llvm/test/CodeGen/Mips/call-site-info-output.ll
35

It's weird to overwrite the same file multiple times. I would just pipe the output of llc straight into FileCheck. You can use \ to split long lines.

ntesic updated this revision to Diff 259866.Apr 24 2020, 5:24 AM

Address comment.
Add changes from https://reviews.llvm.org/D78104.

ntesic marked 2 inline comments as done.Apr 24 2020, 5:26 AM
ntesic added inline comments.
llvm/test/CodeGen/Mips/call-site-info-output.ll
35

I agree. Thanks for the comment.
Also, I moved RUN lines to the top.

vsk added a comment.Thu, May 7, 11:24 AM

This seems to be in great shape. Does it have any dependencies?

llvm/lib/Target/Mips/MipsISelLowering.cpp
3355

nit, this won't feed into doxygen, so the \p can be dropped.

Herald added a project: Restricted Project. · View Herald TranscriptThu, May 7, 11:24 AM
ntesic updated this revision to Diff 263374.Tue, May 12, 12:50 AM
ntesic marked an inline comment as done.

Address comment.
Tests update - use only one !DILocation per scope.

ntesic marked an inline comment as done.Tue, May 12, 12:51 AM
In D78105#2025646, @vsk wrote:

This seems to be in great shape. Does it have any dependencies?

Thanks for the review.
This patch is not dependent on any other.

vsk accepted this revision.Tue, May 12, 1:03 PM

Lgtm! This is a fairly significant change, so please wait for another +1 if you don't mind.

This revision is now accepted and ready to land.Tue, May 12, 1:03 PM
djtodoro accepted this revision.Wed, May 13, 3:06 AM

Lgtm, thanks!

Please make sure you mention the front-end part in the summary (since we merged D78104 into this one; probably the caption here should be something similar to the one from D78104).

I have a comment about the CHECK/PARSER test cases. Otherwise this looks good to me.

llvm/test/CodeGen/Mips/call-site-info-output.ll
35

This comment says that we parse output MIR, but the PARSER commands take IR as input?

RUN: llc -mtriple=mips-linux-gnu -emit-call-site-info %s -stop-after=finalize-isel -o -| FileCheck %s --check-prefix=PARSER

To test MIR parsing, perhaps we can just combine the two RUN lines into something like this?

llc %s -stop-after=finalize-isel -o - | llc -run-pass=finalize-isel -o - | Filecheck %s

And if we keep two separate RUN invocations, the CHECK and PARSER are identical as far as I can tell, so can we combine them to one group then?

ntesic updated this revision to Diff 263951.Thu, May 14, 2:39 AM
ntesic edited the summary of this revision. (Show Details)

Address comments:

  • tests changes
  • patch summary update
ntesic edited the summary of this revision. (Show Details)Thu, May 14, 2:41 AM
ntesic marked 2 inline comments as done.Thu, May 14, 2:44 AM
llvm/test/CodeGen/Mips/call-site-info-output.ll
35

Thanks for pointing out this.
I agree with your suggestions and I have fixed RUN lines.

ntesic marked an inline comment as done.Thu, May 14, 2:45 AM
dstenb accepted this revision.Thu, May 14, 2:59 AM

LGTM. Thanks!

Do you need help with landing this?

llvm/test/CodeGen/Mips/dbg-call-site-info-reg-d-split.ll
7

Nit: emitted

LGTM. Thanks!

Do you need help with landing this?

One of my colleagues will push this.
Thanks for the review.

ntesic updated this revision to Diff 264169.Fri, May 15, 12:19 AM

Address comment - spelling.
Rebase.

ntesic marked an inline comment as done.Fri, May 15, 12:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, May 15, 1:35 AM