Page MenuHomePhabricator

[LiveDebugValues] Emit parameter's entry value
AcceptedPublic

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
aprantl added inline comments.Apr 15 2019, 8:54 PM
lib/CodeGen/AsmPrinter/DwarfExpression.cpp
333 ↗(On Diff #195197)

This should be DW_OP_entry_value if DWARF 5 is requested (or a perhaps GDB tuning)

lib/CodeGen/LiveDebugValues.cpp
450 ↗(On Diff #195197)

Would it be possible to split this part into a separate patch?

djtodoro marked 4 inline comments as done.Apr 15 2019, 11:59 PM
djtodoro added inline comments.
include/llvm/IR/DebugInfoMetadata.h
2529 ↗(On Diff #195197)

I agree with this.

2600 ↗(On Diff #195197)

In LLVM IR I'd prefer using the DWARF 5 DW_OP_entry_value (Or a new LLVM one if we change the semantics). We can still emit the GNU variant, if requested.

We will make it.

According to the DWARF spec an entry value can appear anywhere in the dwarf expression, right?

Yes, but for now we support just basic entry values expressions.

If we only accept one as the first element, we also need a verifier that enforces this.

Sure.

lib/CodeGen/AsmPrinter/DwarfExpression.cpp
333 ↗(On Diff #195197)

Sure.

lib/CodeGen/LiveDebugValues.cpp
450 ↗(On Diff #195197)

IIUC, splitting introduction and production of entry_values makes sense.

djtodoro updated this revision to Diff 195722.Apr 18 2019, 6:14 AM

-Use DW_OP_entry_value from DWARF 5
-Split up introduction and production of entry values

ormris removed a subscriber: ormris.Apr 18 2019, 9:27 AM
aprantl added inline comments.Apr 22 2019, 10:10 AM
lib/CodeGen/LiveDebugValues.cpp
211 ↗(On Diff #196055)

Can you convert this into early-exit form?

if (Var != Other.Var)
  return Var < Other.Var;
...
432 ↗(On Diff #196055)

The combination of TODO and also makes these sentences confusing to me.

454 ↗(On Diff #196055)

factor out VarLocIDs[ID] (.Var.getVar()) for readability?

455 ↗(On Diff #196055)
if (! ...)
  continue
468 ↗(On Diff #196055)

auto &foo = **DMI;

482 ↗(On Diff #196055)

Transfers.push_back({&MI, EntryValDbgMI});

djtodoro marked 5 inline comments as done.Apr 24 2019, 4:42 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
211 ↗(On Diff #196055)

Sure.

454 ↗(On Diff #196055)

Sure.

djtodoro updated this revision to Diff 196431.Apr 24 2019, 5:39 AM

-Addressing comments

aprantl added inline comments.Apr 24 2019, 1:14 PM
lib/CodeGen/LiveDebugValues.cpp
213 ↗(On Diff #196431)

Now we're mixing both styles ;-)

if (Var != Other.Var)
  return Var < Other.Var;
if (Other.Kind != Kind)
  return Other.Kind < Kind;
return Loc.Hash < Other.Loc.Hash;
457 ↗(On Diff #196431)

I'm having trouble making sense of DMI (debug machine instruction?) is there a better name?

956 ↗(On Diff #196431)

Could this be a helper variable or function with a descriptive name that explains what this condition checks for?

djtodoro marked 5 inline comments as done.Apr 25 2019, 8:36 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
213 ↗(On Diff #196431)

Sorry. :)

457 ↗(On Diff #196431)

We can try.

956 ↗(On Diff #196431)

Sure.

djtodoro updated this revision to Diff 196642.Apr 25 2019, 8:40 AM
djtodoro marked 2 inline comments as done.

-Refactor

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
215 ↗(On Diff #199220)

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?

219 ↗(On Diff #199220)

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

298 ↗(On Diff #199220)

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

431 ↗(On Diff #199220)

K -> Kind

598 ↗(On Diff #199220)

IfAvailable ... can this return a nullptr?

935 ↗(On Diff #199220)

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

952 ↗(On Diff #199220)

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
215 ↗(On Diff #199220)

This looks better. Thanks :)

219 ↗(On Diff #199220)

It should be 8.

598 ↗(On Diff #199220)

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
217 ↗(On Diff #199794)

DebugParamSet is fine.

597 ↗(On Diff #199794)

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
885 ↗(On Diff #200035)

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

966 ↗(On Diff #200035)

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

967 ↗(On Diff #200035)

Why can't fragments be entry values?

aprantl added inline comments.May 17 2019, 9:49 AM
lib/CodeGen/LiveDebugValues.cpp
455 ↗(On Diff #200035)

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

950 ↗(On Diff #200035)

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
455 ↗(On Diff #200035)

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.

885 ↗(On Diff #200035)

Sure.

950 ↗(On Diff #200035)

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

966 ↗(On Diff #200035)

Sure. We will add that.

967 ↗(On Diff #200035)

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.Mon, May 27, 5:20 AM

-Refactor test cases a bit
-Rebase

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

LGTM with an extra assertion

lib/CodeGen/LiveDebugValues.cpp
169 ↗(On Diff #201513)

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.Tue, May 28, 9:29 AM
djtodoro updated this revision to Diff 201881.Wed, May 29, 5:47 AM

-Addressing suggestions

djtodoro marked an inline comment as done.Wed, May 29, 5:52 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
169 ↗(On Diff #201513)

Yes, you are right! Thanks!

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

-Rebase tests

dstenb added a comment.EditedWed, Jun 5, 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 :-)