Page MenuHomePhabricator

[DebugInfo][X86] Describe call site values for zero-valued imms
ClosedPublic

Authored by dstenb on Sep 5 2019, 7:29 AM.

Details

Summary

Add zero-materializing XORs to X86's describeLoadedValue() hook in order
to produce call site values.

I have had to change the defs logic in collectCallSiteParameters() a bit
to be able to describe the XORs. The XORs implicitly define $eflags,
which would cause them to never be considered, due to a guard condition
that I->getNumDefs() is one. I have changed that condition so that we
now only consider instructions where a forwarded register overlaps with
the instruction's single explicit define. We still need to collect the implicit
defines of other forwarded registers to remove them from the work list.
I'm not sure how to move towards supporting instructions with multiple
explicit defines, cases where forwarded register are implicitly defined,
and/or cases where an instruction produces values for multiple forwarded
registers. Perhaps the describeLoadedValue() hook should take a register
argument, and we then leave it up to the hook to describe the loaded
value in that register? I have not yet encountered a situation where
that would be necessary though.

Diff Detail

Repository
rL LLVM

Event Timeline

dstenb created this revision.Sep 5 2019, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2019, 7:29 AM
dstenb added a comment.Sep 5 2019, 7:43 AM

I have experimented a bit with making the describeLoadedValue() hook return machine operand objects rather than pointers, as that can simplify the code slightly.

The MachineOperand class only holds POD members, and on x86-64 it is 32 bytes large. That combined with copy elision means that the overhead of returning a machine operand object does not become very large.

I benchmarked that on a 8-thread i7-8650U machine with 32 GB RAM. The benchmark consisted of building a clang 8.0 binary configured with:

-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DLLVM_TARGETS_TO_BUILD=X86 \
-DLLVM_USE_SANITIZER=Address \
-DCMAKE_CXX_FLAGS="-Xclang -femit-debug-entry-values -stdlib=libc++"

The average wall clock time increased by 4 seconds (0.1%), from 62:05 to 62:09. Would that be an acceptable trade-off for slightly simpler code? If so, I can upload a patch for that, and rebase this patch on top of that.

vsk added a comment.Sep 5 2019, 3:41 PM

Thanks for working on this!

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
601 ↗(On Diff #218916)

registers

649 ↗(On Diff #218916)

Consider renaming these vectors with 's/Defs/ForwardedDefs/g'. This would clarify that these defs are both a) attached to I and b) forwarded.

656 ↗(On Diff #218916)

auto Reg : concat<unsigned>(ExplicitDefs, ImplicitDefs) ?

665 ↗(On Diff #218916)

I think this is a reasonable limitation to enforce until we have a clearer picture of how to handle instructions with multiple forwarded defines.

665 ↗(On Diff #218916)

Should this be: I->getNumExplicitDefs() != 1 || ExplicitDefs.empty ?

Thanks for this! As we have discussed before, this is desirable for sure.

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.

+1

dstenb updated this revision to Diff 219041.Sep 6 2019, 12:57 AM
dstenb edited the summary of this revision. (Show Details)

Rebase on top of pointer -> object refactoring.

dstenb updated this revision to Diff 219048.Sep 6 2019, 2:02 AM
dstenb marked 7 inline comments as done.

Address the rest of the comments.

dstenb added inline comments.Sep 6 2019, 2:03 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
649 ↗(On Diff #218916)

I renamed them to the slightly shorter FwdRegDefs to avoid some awkward indentation due to >80 cols.

656 ↗(On Diff #218916)

Neat!

665 ↗(On Diff #218916)

Oh, yes. Thanks for catching that!

This looks good! I'm happy to see that implicit operands can be removed from the work-list this efficiently.

Having ParamLoadedValue contain a MachineOperand object makes sense to me. This would stave off an explosion of ad hoc OneOperand, TwoOperand, etc forever-objects. And you've demonstrated that the overhead is negligible.

Looking forward, I also think it makes sense to change the describeLoadedValue API s.t. it's specific about which loaded value is interesting. That can wait until there's a motivating test case.

+1

+1

vsk accepted this revision.Sep 6 2019, 9:14 AM

Lgtm.

This revision is now accepted and ready to land.Sep 6 2019, 9:14 AM
dstenb added a comment.Sep 8 2019, 5:57 AM

Thanks for the reviews! I'll land these commits shortly.

This revision was automatically updated to reflect the committed changes.