Page MenuHomePhabricator

[CSInfo][NFC] Interpret loaded parameter value separately

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



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

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

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.


This is unused as far as I can tell.


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

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

Nit: curMI -> CurMI


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.

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!


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.

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

What does nextInstrInterpret need to capture via [&]?


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

djtodoro added inline comments.May 7 2020, 11:23 PM

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

djtodoro added inline comments.May 8 2020, 12:24 AM

Some nits included.


MachineFunction *MF


MachineFunction *MF


MachineBasicBlock *MBB

djtodoro added inline comments.May 18 2020, 2:37 AM

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


into static function

ntesic marked 3 inline comments as done.May 21 2020, 6:55 AM
djtodoro added inline comments.May 21 2020, 7:00 AM

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.