Page MenuHomePhabricator

[MS] Emit S_HEAPALLOCSITE debug info in SelectionDAG
ClosedPublic

Authored by akhuang on Apr 24 2019, 5:05 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

akhuang created this revision.Apr 24 2019, 5:05 PM

+@efriedma @bogner Is this SDNode map a good idea? It was the least invasive way we could think of to pass the call site markers down to the MachineInstr.

IIRC @bogner you ran into issues with SDNode replacement and reuse. Is this map keyed by SDNode reasonable, or are we asking for ABA problems? Apologies if I'm misremembering where I heard this story.

Is this SDNode map a good idea?

It's not ideal. I mean, if possible, it would be more clear to store the information as a property of the call node itself. And it's generally not setting a good example to use a map with SDNode pointers rather than some sort of handle. Could you use an MDNodeSDNode here as an operand to the CALL? (You can create one with SelectionDAG::getMDNode().)

The address of an X86ISD::CALL is likely stable, but there isn't really any guarantee; it's just incidental due to the way call nodes are constructed.

Could you use an MDNodeSDNode here as an operand to the CALL? (You can create one with SelectionDAG::getMDNode().)

I tried adding the MDNodeSDNode as an operand, but was running into cases where the MDNode operand disappears (such as when the function has parameters). Is there a good way to deal with this?

rnk added a comment.May 1 2019, 2:18 PM

After discussing this for the last few days, I realize that we're skating on the thin ice that the X86ISD::CALL SDNode doesn't get replaced, but I'm OK relying on that for this feature. In the worst case, some debug info is dropped and we'll have to come back and investigate it later.

the MDNode operand disappears

Disappears at what point, exactly? I can't think of any pass that would drop the node, unless isel itself is dropping it somehow.

we're skating on the thin ice

Yes.

I think it's being dropped in isel as an unused node.

It's getting dropped by isel? Assuming it's still around when Select() is called, you might need to do a little work at that point to attach it to the ScheduleDAG node, I guess?

rnk added inline comments.May 6 2019, 2:57 PM
llvm/include/llvm/CodeGen/SelectionDAG.h
1638–1640 ↗(On Diff #196556)

You should avoid doing two hash lookups: put the result of .find in a variable, and then return the value from the map iterator (Iter->second).

I made some changes to keep the MDNodeSDNode attached to the call, which I mostly did by adding some special cases when operands are MDNodeSDNodes. Since call operands are currently all registers, it seems like it would be less invasive to use a map than to change the convention to include this specific MDNodeSDNode as an operand.

We also want the metadata to be dropped during tail call optimization, which would require some extra work to do here, but works with the map since the call gets replaced there.

akhuang updated this revision to Diff 198500.May 7 2019, 10:57 AM
  • Avoid doing two hash lookups
akhuang marked an inline comment as done.May 7 2019, 1:13 PM
akhuang planned changes to this revision.Jul 19 2019, 2:58 PM

There is now a new patch that attaches the metadata to a call operand-- https://reviews.llvm.org/D65023.

akhuang updated this revision to Diff 211232.Jul 22 2019, 5:22 PM

It seems there is now an SDNode map in SelectionDAG for CallSiteInfo.
I merged the HeapAllocSite map with the CallSiteInfo map so that we don't have
two maps that use SDNodes as keys.

rnk accepted this revision.Jul 30 2019, 12:58 PM

One issue, but otherwise this looks good to me.

llvm/include/llvm/CodeGen/SelectionDAG.h
275 ↗(On Diff #211232)

Use = nullptr here so that any default constructed entries don't appear to have heap alloc sites. This would happen when there is CallSiteInfo for a call site, but no heap alloc site.

This revision is now accepted and ready to land.Jul 30 2019, 12:58 PM
akhuang updated this revision to Diff 212475.Jul 30 2019, 5:12 PM
akhuang marked an inline comment as done.

Add nullptr to struct declaration

This revision was automatically updated to reflect the committed changes.