Page MenuHomePhabricator

[X86] If PreprocessISelDAG reorders a load before a call, make sure we remove dead nodes from the graph
ClosedPublic

Authored by craig.topper on Apr 25 2019, 4:54 PM.

Details

Summary

The reordering can leave at least a dead TokenFactor in the graph. This cause the linearize scheduler to fail with something like the assert seen in PR22614. This is only one of many ways we can break the linearize scheduler today so I can't say for sure that any of the other failures in that bug were caused by this issue.

I took the heavy hammer approach of just running the full RemoveDeadNodes function which will search for dead nodes. We might be able to be more precise and remove exactly the dead TokenFactor or detect that the dead TokenFactor even exists, but I wanted to start with the simplest fix.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Apr 25 2019, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 25 2019, 4:54 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

I worry this is just fix just the dangling nodes as we see them. A brief glance at the AND construction gives us space for this to happen again. I'd much rather we do the removal check unconditionally or leverage something like
DAGUpdateListener to get a more fundamental measure of what nodes may be prunable.

By AND construction, I assume you're referring to turning X86ISD::AND into ISD::AND? What's wrong with that code? It's already removing the node it left dead. Or is there a CSE issue?

I was thinking things pending from the folding optimization in getNode, but I missed that it was the target specific one so it should be fine. That said, I still think it's better to do it unconditionally so it'll be pruned by default with the future additio of Listener-based pruning if that proves too expensive.

By AND construction, I assume you're referring to turning X86ISD::AND into ISD::AND? What's wrong with that code? It's already removing the node it left dead. Or is there a CSE issue?

The AND code is creating target independent ISD::AND from target specific X86ISD::AND so it could be subject to getNode optimization.

Should I make it completely uncondition or make all of the modifications in this loop set the RemoveDeadNodes flag?

I think, make it unconditional.

Remove dead nodes unconditionally

niravd accepted this revision.Apr 30 2019, 7:14 AM

LGTM.

This revision is now accepted and ready to land.Apr 30 2019, 7:14 AM
This revision was automatically updated to reflect the committed changes.