Page MenuHomePhabricator

[MS] Pass S_HEAPALLOCSITE metadata through SelectionDAG

Authored by akhuang on Jul 19 2019, 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.Jul 19 2019, 2:54 PM
rnk added a comment.Jul 22 2019, 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.EditedAug 5 2019, 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.Aug 5 2019, 1:44 PM

clang format

rnk added inline comments.Aug 6 2019, 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.Aug 6 2019, 1:48 PM

Remove tail call metadata; update test

akhuang marked 3 inline comments as done.Aug 6 2019, 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)Aug 6 2019, 2:07 PM
akhuang updated this revision to Diff 214016.Aug 7 2019, 3:15 PM
  • Add clear to the SDNode map when Selection DAG is cleared
akhuang edited the summary of this revision. (Show Details)Aug 7 2019, 3:35 PM
rnk accepted this revision.Aug 7 2019, 3:45 PM

lgtm, let's try again...

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