Page MenuHomePhabricator

[MS] Pass S_HEAPALLOCSITE metadata through SelectionDAG
ClosedPublic

Authored by akhuang on Fri, Jul 19, 2:54 PM.

Details

Reviewers
rnk
Summary

This emits CodeView debug info for heapallocsite by emitting labels around
the call instructions. The metadata is passed through instruction selection by mapping SDNodes to metadata.
https://reviews.llvm.org/D60800 is a previous change to emit the metadata
when FastISel is used.

Related to bug https://bugs.llvm.org/show_bug.cgi?id=38491

This is a recommit of https://reviews.llvm.org/D61105

Event Timeline

akhuang created this revision.Fri, Jul 19, 2:54 PM
rnk added a comment.Mon, Jul 22, 1:09 PM

We discussed this offline, and after looking at rL364516, it seems like the side table might be a better way to go.

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
789–790

I don't think we should assume that any metadata node on a call is a heap alloc site.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
3390–3391 ↗(On Diff #210912)

Given that this is hot code, I think we want to avoid scanning operands if possible. Can we only do this for variadic instructions, like calls?

akhuang updated this revision to Diff 213453.EditedMon, Aug 5, 1:41 PM

The code was adding the metadata to the wrong node in the tail call path; now fixed.

akhuang updated this revision to Diff 213454.Mon, Aug 5, 1:44 PM

clang format

rnk added inline comments.Tue, Aug 6, 12:05 PM
llvm/lib/Target/X86/X86ISelLowering.cpp
4061

Code golf suggestion, saves a hash lookup:

if (CLI.CS)
  if (MDNode *HeapAlloc = CLI.CS->getMetatata("heapallocsite"))
    DAG.addHeapAllocSite(Ret.getNode(), HeapAlloc);
4062

I thought testing showed that MSVC didn't emit these records for tail calls. If we simply do nothing here, we'd skip the S_HEAPALLOCSITE record, and things would keep working, right? Any reason not to do that?

4074

Same suggestion

akhuang updated this revision to Diff 213708.Tue, Aug 6, 1:48 PM

Remove tail call metadata; update test

akhuang marked 3 inline comments as done.Tue, Aug 6, 2:05 PM
akhuang added inline comments.
llvm/lib/Target/X86/X86ISelLowering.cpp
4062

Yeah, not sure why I added it back here. I removed it, but some invalid symbol redefinitions are still occurring.

akhuang edited the summary of this revision. (Show Details)Tue, Aug 6, 2:07 PM
akhuang updated this revision to Diff 214016.Wed, Aug 7, 3:15 PM
  • Add clear to the SDNode map when Selection DAG is cleared
akhuang edited the summary of this revision. (Show Details)Wed, Aug 7, 3:35 PM
rnk accepted this revision.Wed, Aug 7, 3:45 PM

lgtm, let's try again...

This revision is now accepted and ready to land.Wed, Aug 7, 3:45 PM