Page MenuHomePhabricator

[LiveDebugValues] Emit parameter's entry value
ClosedPublic

Authored by djtodoro on Feb 11 2019, 3:50 AM.

Details

Summary

Job of this pass to broadcast DBG_VALUES to the all successive blocks where its preserved location is valid. As a natural way it should emit replacements for clobbered parameters location.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I cherry-picked the patches on top of r359425, and tried out some examples. Doing that, I encountered a case where entry values are not inserted with this patch.

When compiling the following code:

int global;
int foo(int p, int q, int r) {
  global = p + 1;
  asm __volatile ("" : : : "edi", "esi", "edx");
  return 123;
}

using the following clang invocation:

clang -O1 -g -Xclang -femit-param-entry-values -mllvm -stop-after=livedebugvalues

gives the following MIR:

body:             |
  bb.0.entry:
    liveins: $edi

    DBG_VALUE $edi, $noreg, !15, !DIExpression(), debug-location !18
    DBG_VALUE $edi, $noreg, !15, !DIExpression(), debug-location !18
    DBG_VALUE $esi, $noreg, !16, !DIExpression(), debug-location !19
    DBG_VALUE $edx, $noreg, !17, !DIExpression(), debug-location !20
    renamable $edi = nsw ADD32ri8 killed renamable $edi, 1, implicit-def dead $eflags, debug-location !21
    DBG_VALUE $edi, $noreg, !15, !DIExpression(DW_OP_entry_value, 1), debug-location !18
    MOV32mr $rip, 1, $noreg, @global, $noreg, killed renamable $edi, debug-location !22 :: (store 4 into @global, !tbaa !23)
    INLINEASM &"", 1, 12, implicit-def dead early-clobber $edi, 12, implicit-def dead early-clobber $esi, 12, implicit-def dead early-clobber $edx, 12, implicit-def dead early-clobber $df, 12, implicit-def dead early-clobber $fpsw, 12, implicit-def dead early-clobber $eflags, !28, debug-location !27
    $eax = MOV32ri 123, debug-location !29
    RETQ killed $eax, debug-location !29

As seen, there is an entry value location inserted for p after $edi is clobbered by the ADD32ri8 for p + q, but there are no entry value locations inserted for q and r after the inline assembly that clobbers their respective register. Is this perhaps due to some known limitation, or should this be considered a bug? Or have I overlooked something when testing this?

If I comment out the line that assigns the value to global, then LiveDebugValues emits entry value locations after the inline assembly for all three parameters.

dstenb added a comment.May 6 2019, 8:46 AM

I cherry-picked the patches on top of r359425, and tried out some examples. Doing that, I encountered a case where entry values are not inserted with this patch.

I had a look at this. LiveDebugValues::ExtendRanges() stops adding debug value instructions to ParamEntryVals as soon as it encounters a debug value for a variable that has already been seen, as pointed out by the "Insert DEBUG_VALUE instructions that track parameters into ParamEntryVals." block comment.

In this test case we are left with redundant DEBUG_VALUE instructions for p after LiveDebugVariables:

# *** IR Dump After Virtual Register Rewriter ***:                                                                                               
# Machine code for function foo: NoPHIs, TracksLiveness, NoVRegs, NoGlobalPredicates                                                             
Function Live Ins: $edi                                                                                                                          
                                                                                                                                                 
0B  bb.0.entry:                                                                                                                                  
      liveins: $edi                                                                                                                              
      DBG_VALUE $edi, $noreg, !"p", !DIExpression(), debug-location !18; kommentar.c:2:13 line no:2                                              
      DBG_VALUE $edi, $noreg, !"p", !DIExpression(), debug-location !18; kommentar.c:2:13 line no:2
      DBG_VALUE $esi, $noreg, !"q", !DIExpression(), debug-location !19; kommentar.c:2:20 line no:2                                              
      DBG_VALUE $edx, $noreg, !"r", !DIExpression(), debug-location !20; kommentar.c:2:27 line no:2

so that seems to be why entry values are not emitted for q and r, unless I have misunderstood something.

I don't know what it would take to stop LiveDebugVariables from emitting the redundant debug values in the first place. If it is not trivial, or there might perhaps be other cases where we can land in redundant entries, then perhaps this patch should be able to handle those entries?

@dstenb Thanks a lot for this test case! We should handle this as well.

djtodoro updated this revision to Diff 199220.May 13 2019, 1:50 AM

-Handle more cases within LiveDebugValues::ExtendRanges()
-Add one more test
-Rebase

aprantl added inline comments.May 13 2019, 11:49 AM
lib/CodeGen/LiveDebugValues.cpp
223

FYI, I recently learned a neat trick:

return std::tie(Var, Kind, Hash) < std::tie(Other.Var, Other.Kind, Other.Hash);

IIUC, this is equivalent to the above code?

223

How was 6 derived (I'm asking because a non-power-of 2 always looks very deliberate).

302

If you don't want the vector size to leak into the function, you can use a SmallVectorImpl<MachineInstr *>

440

K -> Kind

609

IfAvailable ... can this return a nullptr?

992

I think the middle condition is redundant? Or do we expect SP to be 0?

1009

return std::any_of()

djtodoro marked 6 inline comments as done.May 16 2019, 5:12 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
223

This looks better. Thanks :)

223

It should be 8.

609

Yes, thanks.

djtodoro updated this revision to Diff 199794.May 16 2019, 5:14 AM

-Address suggestions
-Rename ParamEntryVals into DebugEntryVals
-Avoid entry values on a frame reg and fragments (found those cases with better verifier)

aprantl added inline comments.May 16 2019, 3:41 PM
lib/CodeGen/LiveDebugValues.cpp
224

DebugParamSet is fine.

610

if (auto *TPC = getAnalysisIfAvailable<TargetPassConfig>())

djtodoro updated this revision to Diff 200035.May 17 2019, 6:38 AM

-Address suggestions

aprantl added inline comments.May 17 2019, 9:44 AM
lib/CodeGen/LiveDebugValues.cpp
943

Can you land this as an NFC refactoring (no need for a separate phab review?)

1025

We need a longer comment here that explains why each of these conditions are necessary and what is being checked for.

1026

Why can't fragments be entry values?

aprantl added inline comments.May 17 2019, 9:49 AM
lib/CodeGen/LiveDebugValues.cpp
467

This is going into (low) n^2 territory here. Should we make DebugEntryVals a SmallPtrSet to avoid catastrophic behavior for functions with many arguments?

1009

What does "New" mean here?

djtodoro marked 5 inline comments as done.May 20 2019, 6:58 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
467

Mmm, we see.. But we will need another set for caching parameters debug variables, since we consider a parameters DBG_VALUE as a "new" if didn't encountered a debug variable from it.

943

Sure.

1009

It means that a parameter is not collected yet inside the DebugEntryVals. We should name it IsParamEntryValueCollected or something like that.

1025

Sure. We will add that.

1026

Fragments can be entry values as well, but don't support it in this initial set of patches.
We will leave a TODO for that here.

djtodoro updated this revision to Diff 200489.May 21 2019, 6:44 AM

-Use map instead of vector for storing debug entry values
-TODO: Land a separate patch with renaming DMI-->DebugInstr

djtodoro updated this revision to Diff 200957.May 23 2019, 6:28 AM

-Addressing comments
-Avoid indirect dbg_values (in addition to SP and FP locations) in order to avoid memory locations as entry values locations

TODO: Rebase this on top of NFC D62295

djtodoro updated this revision to Diff 201513.May 27 2019, 5:20 AM

-Refactor test cases a bit
-Rebase

aprantl accepted this revision.May 28 2019, 9:29 AM

LGTM with an extra assertion

lib/CodeGen/LiveDebugValues.cpp
170

This looks dangerous.. are entry values somehow implicitly also register values? Iguess besed on the other patches the answer is yes, but that also means that we must assert((!EntryValueKind || RegNo) && "entry values must be register locations")

This revision is now accepted and ready to land.May 28 2019, 9:29 AM
djtodoro updated this revision to Diff 201881.May 29 2019, 5:47 AM

-Addressing suggestions

djtodoro marked an inline comment as done.May 29 2019, 5:52 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
170

Yes, you are right! Thanks!

djtodoro updated this revision to Diff 202419.May 31 2019, 5:08 AM

-Rebase tests

dstenb added a comment.EditedJun 5 2019, 5:40 AM

Have you measured what the effect the emission of entry value locations has on the "scope bytes covered" statistics? I guess that it can increase quite a bit (especially later on if we start describing non-parameter variables using parameter entry values)? If so, if the caller(s) do not have call site information, it will still not be able to print the variable. Would it make sense, and would it be possible, to introduce a "scope bytes covered without entry values" statistic, or some other statistic, which helps you assess how large part of the locations that rely on entry values?

@dstenb Thanks for the comment!

Yes, we are constantly measuring debug location statistics. It is possible (and it makes sense) to extend the existing location statistic reporting, within LLVM, by avoiding locations that are entry values (we are actually working on that).

We will share the numbers.

@dstenb Thanks for the comment!

Yes, we are constantly measuring debug location statistics. It is possible (and it makes sense) to extend the existing location statistic reporting, within LLVM, by avoiding locations that are entry values (we are actually working on that).

We will share the numbers.

Okay, thanks!

Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 20, 6:36 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

@aprantl This still looks OK after the rebasing? :)

A rebase shouldn't affect the spirit of the change :-)

This revision was automatically updated to reflect the committed changes.
djtodoro updated this revision to Diff 207287.Mon, Jul 1, 6:29 AM

-Avoid a single location represented as an entry value (+ add a test for that)
-Allow a debug entry value after a first terminator

djtodoro updated this revision to Diff 207290.Mon, Jul 1, 6:38 AM

Nit: Use DWARF 5 in a test case.

djtodoro reopened this revision.Mon, Jul 1, 6:44 AM

Due to very latest patches we came up with situations when we need to avoid single location represented as an entry value.
Also, having in mind the debug entry value is special case of the 'DBG_VALUE' instruction, we found it is OK in some situations to generate it after a first terminator.
Is this still OK to go ?

This revision is now accepted and ready to land.Mon, Jul 1, 6:44 AM
djtodoro updated this revision to Diff 207294.Mon, Jul 1, 7:02 AM

-Clarify in the test the entry value coming from a location list

aprantl added inline comments.Mon, Jul 1, 10:09 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1267 ↗(On Diff #207294)

Can you change this comment from describing what the code does (which is fairly obvious), to why it does it?

thanks!

dstenb added inline comments.Tue, Jul 2, 1:14 AM
lib/CodeGen/LiveDebugValues.cpp
1016

I'm sorry for the late comment, but in the future I guess this could be extended so that entry_values are emitted for locals that are expressed in terms of parameters, as GCC does?

For example:

int foo(int arg) {
  int local = arg + 3;
  clobber();
  return 0;
}
00000039 0000000000000004 000000000000000d (DW_OP_breg5 (rdi): 3; DW_OP_stack_value)
0000004e 000000000000000d 0000000000000018 (DW_OP_GNU_entry_value: (DW_OP_reg5 (rdi)); DW_OP_plus_uconst: 3; DW_OP_stack_value)
00000066 <End of list>

As far as I can tell, we don't tag non-parameter variables as NotModified, but can Clang later on be extended to mark them as such?

Can you change this comment from describing what the code does (which is fairly obvious), to why it does it?

@aprantl Sure. Thanks for the comment!

I'm sorry for the late comment, but in the future I guess this could be extended so that entry_values are emitted for locals that are expressed in terms of parameters, as GCC does?

@dstenb Thanks for your attention and comments. Sure, that is the plan. This is initial set of patches that covers just non-modified function parameters. Even the initial patch within the GCC was handling just this situation. The ways for improving this (using benefits of the debug entry values) are extending the support for modified parameters (if its modification could be described as an expression over its entry value), supporting local variables, etc.

As far as I can tell, we don't tag non-parameter variables as NotModified, but can Clang later on be extended to mark them as such?

Yes, it can be extended.

In addition, I did not forget about sharing the info about the debug location statistic measuring. :)
The 'llvm-dwarfdump' does calculate the debug location statistics, but maybe we could think of reporting it with more information, since the debug location info is such important debug info. Please find a proposal for having a separate tool that will calculate only the debug location statistics on my github (https://github.com/djolertrk/llvm-locstats) and let me know if it can be useful for us. Potentially, we could add more options, functionalities, etc..

dstenb added a comment.Tue, Jul 2, 4:09 AM

Can you change this comment from describing what the code does (which is fairly obvious), to why it does it?

@aprantl Sure. Thanks for the comment!

I'm sorry for the late comment, but in the future I guess this could be extended so that entry_values are emitted for locals that are expressed in terms of parameters, as GCC does?

@dstenb Thanks for your attention and comments. Sure, that is the plan. This is initial set of patches that covers just non-modified function parameters. Even the initial patch within the GCC was handling just this situation. The ways for improving this (using benefits of the debug entry values) are extending the support for modified parameters (if its modification could be described as an expression over its entry value), supporting local variables, etc.

As far as I can tell, we don't tag non-parameter variables as NotModified, but can Clang later on be extended to mark them as such?

Yes, it can be extended.

Okay, thanks for the clarification! Would it perhaps make sense to add a small sentence about that in those TODOs?

Would it perhaps make sense to add a small sentence about that in those TODOs?

@dstenb Sure! Thanks!

djtodoro updated this revision to Diff 207543.Tue, Jul 2, 6:27 AM

-Add support for entry values that are complex variables address (that avoids entry value not be safe for a single locations)
-Add a TODO for supporting local variables that are expressed in terms of parameters entry values

Is this OK to go ?

dstenb added a comment.Thu, Jul 4, 7:07 AM

The 'llvm-dwarfdump' does calculate the debug location statistics, but maybe we could think of reporting it with more information, since the debug location info is such important debug info. Please find a proposal for having a separate tool that will calculate only the debug location statistics on my github (https://github.com/djolertrk/llvm-locstats) and let me know if it can be useful for us. Potentially, we could add more options, functionalities, etc..

That seems like a useful tool!

Did you consider what should be done with llvm-dwarfdump's "scope bytes covered" statistics?

lib/CodeGen/LiveDebugValues.cpp
170

I think this should be Kind != EntryValueKind?

djtodoro marked an inline comment as done.Thu, Jul 4, 7:49 AM

The 'llvm-dwarfdump' does calculate the debug location statistics, but maybe we could think of reporting it with more information, since the debug location info is such important debug info. Please find a proposal for having a separate tool that will calculate only the debug location statistics on my github (https://github.com/djolertrk/llvm-locstats) and let me know if it can be useful for us. Potentially, we could add more options, functionalities, etc..

That seems like a useful tool!

Thanks a lot for taking a look!

Did you consider what should be done with llvm-dwarfdump's "scope bytes covered" statistics?

I think that it sounds reasonable to add an additional field there, something like "non-entry-val scope bytes covered".

lib/CodeGen/LiveDebugValues.cpp
170

Oh, nice catch! Thanks!

djtodoro updated this revision to Diff 208042.Thu, Jul 4, 7:51 AM

-Fix the assertion

dstenb added a comment.Fri, Jul 5, 1:03 AM

Did you consider what should be done with llvm-dwarfdump's "scope bytes covered" statistics?

I think that it sounds reasonable to add an additional field there, something like "non-entry-val scope bytes covered".

Just to clarify, as the emission of entry values is behind a non-driver flag I'm not opposed to landing these patches without changing llvm-dwarfdump, so please don't let me stand in the way. I think that a "non-entry-val scope bytes covered" field would be helpful, but someone with more say in the debug info area than me should decide that.

Did you consider what should be done with llvm-dwarfdump's "scope bytes covered" statistics?

I think that it sounds reasonable to add an additional field there, something like "non-entry-val scope bytes covered".

Just to clarify, as the emission of entry values is behind a non-driver flag I'm not opposed to landing these patches without changing llvm-dwarfdump, so please don't let me stand in the way. I think that a "non-entry-val scope bytes covered" field would be helpful, but someone with more say in the debug info area than me should decide that.

@dstenb We could land this right now, and the patch for the 'llvm-dwarfdump' stats could be a separate patch. Do you agree?
Also, in the thread we could discuss about the new tool I shared.

djtodoro updated this revision to Diff 208321.Mon, Jul 8, 12:07 AM

-Nit in a comment

dstenb added a comment.Mon, Jul 8, 3:01 AM

@dstenb We could land this right now, and the patch for the 'llvm-dwarfdump' stats could be a separate patch. Do you agree?

Yes, that sounds good to me!

lib/CodeGen/LiveDebugValues.cpp
170

Also, I think we want to move the assertion out of this if statement? We want the check for all types of operands, not just for those that we have seen are register. (Currently, the RegNo check, with the assertion placed inside that if statement should mean that the assert can never fail.)

djtodoro updated this revision to Diff 208365.Mon, Jul 8, 4:26 AM

-Remove redundant code
-Fix the debug entry value assertion from the VarLoc constructor

djtodoro marked an inline comment as done.Mon, Jul 8, 4:29 AM

Yes, that sounds good to me!

@dstenb Thanks!

lib/CodeGen/LiveDebugValues.cpp
170

Yes, it will never fail.
Also, the code looks much cleaner right now! Thanks!

This revision was automatically updated to reflect the committed changes.