[PATCH] D43198: [X86] Fix Topological NodeId Ordering violation in Load-Op-Store fusion.

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

After discussion with Nirav --

We now think this is not sufficient to ensure the correct ordering in all cases, because this sort of situation can _probably_ come up in other places too where nodes are merged.

The property that hasPredecessorHelper requires (when Topological Sort is enabled) is that all predecessors have a nodeId less than the nodeId of all successors, recursively. But, this can be violated any time two nodes are merged where the earlier node in the merge pattern has successors outside of the pattern, which have not yet been selected, and the latter node in the pattern has another predecessor.

To make the property sound, any such additional successor nodes would need to have their nodeIds invalidated. (Invalidated means nodeId == -1)

This can only be an issue when there are successors of interior nodes in a merge pattern -- and should only happen where the caller has checked for cycles before doing the merge with hasPredecessor*. When it happens, nodeIds between the min and max in the pattern may need to be invalidated. The typical caller to hasPredecessor is through IsLegalToFold, so callers to that need to be examined too.

The change in this review does try to check the above, but:

1. Only for the X86 special case. The generic case can have the same happen, although generally only via chain outputs from the interior nodes.
2. The invalidation function here stops upon seeing a -1, but it's possible (afaict) for a node with a nodeId -1 to have a successor which has a valid nodeid.

So:
1a. Need to call the invalidation from the generic code in SelectionDAGIsel when merging input chains.
1b. Look at the other callers of IsLegalToFold to make sure they're okay too (some may already be covered by 1a)

2. Need to iterate over the node list, invalidating all nodes between the minimal id and the maximal id in the pattern that are successors to an already-selected node.
3. Need to figure out how to check this property in a way that'll catch future errors.

Hopefully the above is correct. =)


Repository:
  rL LLVM

https://reviews.llvm.org/D43198