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
djtodoro edited the summary of this revision. (Show Details)Apr 15 2019, 8:50 AM

-Rebase
-Follow-up with the new approach

aprantl added inline comments.Apr 15 2019, 8:54 PM
include/llvm/IR/DebugInfoMetadata.h
2529 ↗(On Diff #195197)

The more flags we add these bool parameters get less and less safe, since the order isn't typesafe. We should probably think about also using an unsigned Flag parameter that is WithDeref | WithEntryValue.

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.

According to the DWARF spec an entry value can appear anywhere in the dwarf expression, right? If we only accept one as the first element, we also need a verifier that enforces this.

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
451

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
451

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
213

Can you convert this into early-exit form?

if (Var != Other.Var)
  return Var < Other.Var;
...
433

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

457

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

458
if (! ...)
  continue
471

auto &foo = **DMI;

485

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
213

Sure.

457

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
214

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;
462

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

1009

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
214

Sorry. :)

462

We can try.

1009

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
218

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?

218

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

296

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

434

K -> Kind

605

IfAvailable ... can this return a nullptr?

988

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

1005

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
218

This looks better. Thanks :)

218

It should be 8.

605

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
219

DebugParamSet is fine.

606

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

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

-Address suggestions

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

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

1021

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

1022

Why can't fragments be entry values?

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

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

1005

What does "New" mean here?

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

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.

939

Sure.

1005

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

1021

Sure. We will add that.

1022

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.Tue, May 21, 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.Thu, May 23, 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
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.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
170

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!