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

[Amaury SECHET via Phabricator via llvm-commits]

deadalnix added a comment.

Hi @craig.topper ,

First about the kind of code I try to get to have better codegen, it's mostly about large integer manipulations. I already added a fair amount of reduced test cases in addcarry.ll/subcarry.ll . I'm at a stage where the pattern I have to work with are somewhat deep, see D57302 <https://reviews.llvm.org/D57302> for an example. These patterns do not do anything useful if other transform cannot pick up from whee they left.

Hi @niravd ,

I wanted to collect some performance data today, but I ran into some problem to get to generate a large bc file of some existing program as LTO seems to be broken on my end for some reason. I did explore the idea of adding more descendant, for instance sext/zext seems to be valuable to punch through. But it seems to me like you would want to add more and more of these over time, and you'd end up with a complicated version of what we have here which misses opportunities.

You raise a good point though. This should only run iteration n if iteration n - 1 actually did change the DAG. You'd expect that the code shouldn't run 3 times that often, but in practice it does because there are a lot of transform that do A -> B -> A . This is why I limited to 3 and not anything more. I do think that investigating these and removing them over time is probably preferable.

> I suggested changing the key node in the such transforms so they were either the earliest or a user of the earliest node so DAG changes would happen before the transform was no longer considered.

I do not think this is a very realistic path forward as there are numerous transform looking 2+ deep. As you rightly point out anything SimplifyDemandedBits based does so for instance.

I think as first step we can hide this behavior behind some flag which default to not doing the transform until we can tune things a bit more and figure out in what cases we want to do this? Would that be acceptable to you?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D33587