This emits labels around heapallocsite calls in SelectionDAG.
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?
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
|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.
One issue, but otherwise this looks good to me.
|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.