Page MenuHomePhabricator

[MS] Pass S_HEAPALLOCSITE metadata through SelectionDAG

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



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. is a previous change to emit the metadata
when FastISel is used.

Related to bug

This is a recommit of

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.


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

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

Code golf suggestion, saves a hash lookup:

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

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?


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.

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