Page MenuHomePhabricator

[LiveDebugValues] Emit parameter's entry value
Needs ReviewPublic

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
wolfgangp added inline comments.Feb 11 2019, 6:16 PM
lib/CodeGen/LiveDebugValues.cpp
318

This type is used in several locations. Perhaps we could have a using declaration
for it and use a more compact name?

831

This is invariant in the for-loop, no? Perhaps it should be pulled ahead of it?

1004

I think this comment needs perhaps some rephrasing. Something like:
Enter DEBUG_VALUE instructions that track parameters into ParameterEntryVals. When we encounter a variable that we have already entered, we assume that we have found all parameter
entry locations and stop.

1006

some spelling errors: wheter -> whether, vraible->variable, it->is

Suggested wording:
We check whether MI is inlined in order to deduce whether the variable that it tracks
comes from a different function. If that is the case we can't track its entry value.

@wolfgangp Thanks a lot for your comments! It has been addressed.

djtodoro updated this revision to Diff 186452.Feb 12 2019, 6:40 AM
djtodoro updated this revision to Diff 186613.Feb 13 2019, 2:45 AM
  • Rename: VariableNotChanged ===> ArgumentNotModified
  • Refactor a test case
dstenb added a subscriber: dstenb.Feb 13 2019, 1:19 PM
dstenb added inline comments.
lib/CodeGen/LiveDebugValues.cpp
445–447

Should this use Expr->isEntryValue()? However, the implementation of that function in D58041 only checks if the first element is the operation:

bool isEntryValue() const {
    return getNumElements() > 0 && getElement(0) == dwarf::DW_OP_GNU_entry_value;
}

Is that correct, or should it look through all elements like is done here? Or is there some reason for why they should be disjunct? If so, could you add a comment here?

NikolaPrica added inline comments.Feb 14 2019, 3:05 AM
lib/CodeGen/LiveDebugValues.cpp
445–447

You are right! It should be isEntryValue()! Thanks!

djtodoro updated this revision to Diff 187190.Feb 17 2019, 10:41 PM
  • Use isEntryValue() from DIExpression
dstenb added inline comments.Feb 18 2019, 5:06 AM
lib/CodeGen/LiveDebugValues.cpp
445–447

Okay, thanks for the clarification. So DW_OP_GNU_entry_value will only be located in the first element then?

djtodoro marked an inline comment as done.Feb 19 2019, 12:14 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
445–447

In the current design, yes.
DIExpression will always have entry value DWARF operand as first element.

djtodoro updated this revision to Diff 195197.Apr 15 2019, 8:50 AM
djtodoro edited the summary of this revision. (Show Details)

-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
457

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
457

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.Mon, Apr 22, 10:10 AM
lib/CodeGen/LiveDebugValues.cpp
213–218

Can you convert this into early-exit form?

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

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

463

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

464
if (! ...)
  continue
477

auto &foo = **DMI;

491

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

djtodoro marked 5 inline comments as done.Wed, Apr 24, 4:42 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
213–218

Sure.

463

Sure.

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

-Addressing comments

aprantl added inline comments.Wed, Apr 24, 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;
468

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

1015

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.Thu, Apr 25, 8:36 AM
djtodoro added inline comments.
lib/CodeGen/LiveDebugValues.cpp
214

Sorry. :)

468

We can try.

1015

Sure.

djtodoro updated this revision to Diff 196642.Thu, Apr 25, 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.Mon, May 6, 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.Mon, May 13, 1:50 AM

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

aprantl added inline comments.Mon, May 13, 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

611

IfAvailable ... can this return a nullptr?

994

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

1011

return std::any_of()

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

This looks better. Thanks :)

223

It should be 8.

611

Yes, thanks.

djtodoro updated this revision to Diff 199794.Thu, May 16, 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.Thu, May 16, 3:41 PM
lib/CodeGen/LiveDebugValues.cpp
224

DebugParamSet is fine.

612

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
945

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

1027

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

1028

Why can't fragments be entry values?

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

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

1011

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
469

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.

945

Sure.

1011

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

1027

Sure. We will add that.

1028

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