[PATCH] D33587: [DAGCombine] Do several rounds of combine.

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added a comment.

@RKSimon Most of these case aren't because node are not added to the worklist, but because of pattern that are somewhat deep - such as anything depending on KnownBits . Consider the following DAG:

  C
  |
  B
  |
  A

Now imagine node A is visited and no combine was found. Then B and C are visited and C is transformed into D:

  D
  |
  B
  |
  A

In this scenrio, A was already visited and isn't added back to the worklist, only B is. We could recursively add uses of uses to the worklist, but it turns out this is not very efficient as it require to add half of the DAG on average back in the worklist everytime a combine is done. Plus it wouldn't catch cases of combine based on getNodeIfExists and other various cases. Simply adding direct uses catches the most common cases, and having an extra pass over all nodes catches everything else, and is more economical as long as > 2 combine are node (on average).

As for performance (I assume you meant compile time performance impact) here are the time I get for a test suite run:

Without this patch:

  real    2m41.665s
  user    26m33.900s
  sys     2m44.728s

With this patch:

  real    2m42.729s
  user    26m45.772s
  sys     2m42.796s

It doesn't looks like the impact is that significant and it seems worth it to me.


https://reviews.llvm.org/D33587