[PATCH] D64174: [DAGCombine] Do several rounds of combine for addcarry nodes.

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added a comment.

SO I came up with a plan to get things processed in topological order. However, as mentioned previously, this is indeed leading into a world of pain. I have a prototype, but it causes numerous regression that I want to investigate.

The general outline is as follow:
1/ Remove as much as possible of the manual management of the worklist. It is not realistic to expect every combine for every opcode + target specific combines to be able to operate the worklist consistently, so utility method to help them do so has to be added. Fortunately, often, this management is not doing anything useful at all and can simply be removed.
2/ Modify the utility methods that are expected to actually manipulate the worklist to mostly preserve topological order. It turn out it is not too difficult to have them do a good enough job.
3/ Do a topological sort at the beginning of the process, before adding all the nodes to the worklist.
4/ Modify the logic that adds the argument of a node to the worklist in case they where not present already to do lazy fixup when required to keep the processing topological. Because of 2 and 3, this ends up to be a lightweight process.

As it turns out, many transform done by DAGCombiner are dependent on the processing order. Because this order is not specifically defined, this means such transform are unreliable in practice, and indeed many do break when the processing order is modified. This is another reason why 2 and 3 are useful, as they allow to move gradually instead of producing one giant diff of death. The opposite also happens, where pattern that were not picked upon before now are being exploited.

I already started to execute step 1 of this plan. If people are happy with taking that path, I'd be happy to push that effort forward. I could do with some help to investigate the regression, as I'm not an expert in every single backends.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64174/new/

https://reviews.llvm.org/D64174