This is an archive of the discontinued LLVM Phabricator instance.

[CSInfo][NFC] Interpret loaded parameter value separately
ClosedPublic

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

Details

Summary

Refactor of DwarfDebug.cpp code.

Currently, collectCallSiteParameters method searches for instructions which load values into
registers used for parameters passing. Previously, interpretation of those values, loaded by one such
instruction, was implemented inside collectCallSiteParameters method.

This patch moves the interpretation code from collectCallSiteParameters method
into a separate static method named interpretValue. New method is called from
collectCallSiteParameters to process each instruction from targeted instruction scope.

Currently collectCallSiteParameters searches for loaded parameter value among instructions
which precede the call instruction, inside the same basic block.
When needed, new method (interpretValue) could be used for searching any instruction scope.

This is preparation for search of parameter value, loaded inside call delay slot.

Diff Detail

Event Timeline

ntesic created this revision.Apr 14 2020, 6:38 AM
dstenb added inline comments.Apr 14 2020, 10:05 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
691–692

Personal opinion, but I instinctively interpret the return value from interpretValue as to whether or not the value was described, not whether or not we are done describing all parameters. Perhaps it might be worth moving those conditions out to this loop instead, and not have interpretValue return anything?

And to avoid code duplication for the upcoming delay slot patch, I guess we can use a lambda for those conditions.

dstenb added inline comments.Apr 14 2020, 10:23 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
645

As I assume the DIExpression lookup is quite cheap, perhaps we should consider getting it inside this function instead of passing it as a parameter.

646

This is unused as far as I can tell.

647

As this is only used as a temporary container inside this function, and is cleared before returning, maybe it should be renamed to something to indicate that, e.g. something with Tmp.

ntesic updated this revision to Diff 259533.Apr 23 2020, 5:12 AM

@dstenb
Thanks for the review.
I agree with all suggestions.

ntesic marked 4 inline comments as done.Apr 23 2020, 5:14 AM
dstenb added inline comments.Apr 27 2020, 4:58 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
641–642

Nit: curMI -> CurMI

695–697

Sorry, but I don't understand what it means to skip "NOP description" in this context.

Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 4:58 AM
ntesic updated this revision to Diff 260577.Apr 28 2020, 3:05 AM

Address comments.

ntesic marked 3 inline comments as done.Apr 28 2020, 3:07 AM
ntesic added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
695–697

The bad comment was here by mistake. Thanks.

ntesic marked an inline comment as done.Apr 28 2020, 3:08 AM
dstenb accepted this revision.Apr 30 2020, 4:06 AM

One minor comment about singular/plural phrasing. Otherwise this looks good to me!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
642

As an instruction may produce multiple register values, maybe this should be called interpretValues, and the comment above updated to reflect that?

This revision is now accepted and ready to land.Apr 30 2020, 4:06 AM
ntesic updated this revision to Diff 261821.May 4 2020, 7:55 AM

Address comment.

ntesic marked 2 inline comments as done.May 4 2020, 7:56 AM
ntesic added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
642

Yes, It makes more sense. Thanks.

ntesic marked an inline comment as done.May 4 2020, 7:57 AM
vsk added inline comments.May 7 2020, 11:15 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
691–692

What does nextInstrInterpret need to capture via [&]?

742

nit, the parens around *I aren't needed.

djtodoro added inline comments.May 7 2020, 11:23 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
691–692

Also the getForwardingRegsDefinedByMI does not need to capture via [&].

djtodoro added inline comments.May 8 2020, 12:24 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
691–692

Some nits included.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
647–649

MachineFunction *MF

742

MachineFunction *MF

750

MachineBasicBlock *MBB

djtodoro added inline comments.May 18 2020, 2:37 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
691–692

Sorry for the confusion.
After the conversation from D79616, I realized that default reference capturing is recommended by the LLVM Style Guide when lambda does not escape its context.

But anyway, this lambda (nextInstrInterpret) captures by reference but it does not use any data from the function, so the [&] could be deleted. Furthermore, maybe nextInstrInterpret could be factored out into a static function? I am OK with either way.
@vsk WDYT?

In addition:

nextInstrInterpret -> interpretNextInstr ?

ntesic updated this revision to Diff 265495.EditedMay 21 2020, 6:49 AM

Address comments.
Transform lambda

nextInstrInterpret

into static function

interpretNextInstr
ntesic marked 3 inline comments as done.May 21 2020, 6:55 AM
djtodoro added inline comments.May 21 2020, 7:00 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
739

clang-format please

ntesic updated this revision to Diff 266024.May 25 2020, 8:06 AM

Clang format.

ntesic marked an inline comment as done.May 25 2020, 8:06 AM
djtodoro accepted this revision.May 27 2020, 4:13 AM

Thanks, lgtm. Please wait for others (for another day at least) to give their final comments.

This revision was automatically updated to reflect the committed changes.