This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Do several rounds of combine for nodes using SimplifyDemandedBits.
Needs ReviewPublic

Authored by deadalnix on Jan 28 2019, 7:08 PM.

Details

Summary

SimplifyDemandedBits can explore severallevels of the DAG docompute its result. This means that a transformation in the DAG that isn't immediately related to a node might affect it.

We therefore add these nodes to the DeepPatternNodes set so that they can be processed again in case the DAG is modfied.

In the future, other node may be added to the set as need arises.

This is a variation on D33587 that should be more lightweight as it focuses the extra work on node that are actually likely to benefit from it rather than the whole DAG.

Diff Detail

Event Timeline

deadalnix created this revision.Jan 28 2019, 7:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 6:19 PM
niravd added a comment.Feb 5 2019, 7:59 AM

I've been looking at this closely and it's descending in a chain of cleanups, after which I'm hoping we can do this in a more complete way.

I'm currently working on fixing the issue with SelectionDAG where we will leave unused nodes all around until they are cleaned up later in the combine pass or the next pass entirely. During the transient various optimizations relying on single use are disabled. This is another part of why the DAGCombiner fails to run to exhaustion of peephole optimizations. I suspect but have not verified that this a lot of the reason why each iteration makes improvements. I hope to have this up for review in a day or two.

My concern about this approach is that the DeepPatternNodes happening at the end of the iteration (and the multiple iteration) is partially hiding this issue. It'd be much more satisfying if we were adding this directly to the worklist and being handled immediately.

We also need to go a deeper than what we traverse SimplifyDemandedBits. At least we must add the users as well. Adding a variant to do of this catches a bunch of additional cases.

xbolva00 resigned from this revision.Mar 2 2019, 12:38 PM
arsenm resigned from this revision.Feb 13 2020, 4:49 PM

@deadalnix Is this still relevant or will D127115 replace it?

Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2022, 5:34 AM

This didn't look like it was getting much traction. Processing things in topological order should provide a good chunk of the value, so we should revisit later.

chfast added a subscriber: chfast.Aug 23 2022, 12:20 AM
chfast added inline comments.Aug 23 2022, 12:33 AM
test/CodeGen/X86/shift-double.ll
294

Is not creating shld the expected result?

test/CodeGen/X86/vector-truncate-combine.ll
1

No changes in this file.

This patch is so old I wouldn't recommend looking for regressions - hopefully D127115 will make it redundant entirely