This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Transitively copy NodeExtraInfo on RAUW
ClosedPublic

Authored by melver on Feb 23 2023, 2:43 PM.

Details

Summary

During legalization of the SelectionDAG, some nodes are replaced with
arch-specific nodes. These may be complex nodes, where the root node no
longer corresponds to the node that should carry the extra info.

Fix the issue by copying extra info to the new node and all its new
transitive operands during RAUW. See code comments for more details.

This fixes the remaining pcsections-atomics.ll tests on X86.

Depends on D144676

Diff Detail

Event Timeline

melver created this revision.Feb 23 2023, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 2:43 PM
melver requested review of this revision.Feb 23 2023, 2:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 2:43 PM
melver added inline comments.Feb 23 2023, 2:50 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
12202

RFC: Is there some other property of the DAG and the nodes that could be exploited to make this more efficient?

dvyukov accepted this revision.Feb 23 2023, 11:00 PM

The code looks sane to me. But I never looked at this part of the code before.
Wonder if this also fixes debug info?

This revision is now accepted and ready to land.Feb 23 2023, 11:00 PM

The code looks sane to me. But I never looked at this part of the code before.
Wonder if this also fixes debug info?

Debug info is handled differently. It's copied explicitly everywhere (incl. arch code), and attached to new nodes explicitly. While debug info and pcsections propagation is done similarly outside of SelectionDAG (BuildMI+MIMetadata for MachineInstrs will do it correctly for both), within SelectionDAG we have stashed pcsections (and some other things) in "extra info" to not increase complexity and cost since it's used infrequently. Therefore, the way extra info is copied is unique to SelectionDAG and different from debug info as well.

It might fix other users of NodeExtraInfo though, such as CallSiteInfo, HeapAllocSite, NoMerge - although it looks like the first 2 only apply to calls, and these don't seem to be replaced with more complex nodes (AFAIK?).

This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Mar 1 2023, 5:21 PM

Hi @melver, we saw a large compile time increase in one of our internal tests which I bisected back to your change. I have filed issue #61108 for the problem, can you take a look?