Rebase and ping.

Fix erroneously inverted condition and ensuring previously broken tests are now passing again.

Add test for vector - but nothing changes for them (see rL374043 ) and use isSubsetOf and getAllOnesValue.

Use isConstOrConstSplat and getAPIntValue

Rebase on top of NFC changes

Ping ?

Add tests for the commutative case.
Precommit tests.

Tighten the checks before creating the node.

• Avoid creating the shift node when not stricly required.
• Add negative tests.

I looked at the problem that @spatel ran into. It is not applicable to DAGCombiner, because the DAG is processed only once rather than in a loop as long as it is modified. I wouldn't be possible to change it to work like InstCombine does as there are a ton of A -> B -> A type of tranforms. That being said, it's not a good reason to add more, so it's worth looking into improving this if possible.

In D66882#1649078, @lebedev.ri wrote:

Pattern look good to me, but i recall having disscussion in https://reviews.llvm.org/D64275#inline-576684
Granted, that is InstCombine, but what happens here if we create instruction but then fail to fold?

IndexC => ExtrIndex

Upadte the doc comment to make it clear that the Mask argument can be modified.

@nathanchance Thanks for this. Having a test case will help tremendously. Sorry for the inconvenience.

Rebase on top of D66804

In D66718#1645956, @lebedev.ri wrote:

Looks like the link was incorrect, this didn't auto-close?

Because the order in which nodes are traversed is not reliable, it is very hard to come up with a test for this. Ultimately, this is part of a larger body of work that intend to ensure nodes are processed in topological order by DAGCombiner. Even if they do not change the codegen, doing these changes are useful as they reduce the pressure on the logic that maintains the topological order.

Add TODOs

Restore calls that are not tested.

Comments amrked as done.

Ok, I found a handful of cases that are not coevered by the test suite. I'll investigate if adding test coverage is possible, if it is do so. If not, then I'll remove any modification from that diff.

In D66537#1644303, @RKSimon wrote:

Please can you confirm that all these cases are covered by existing code coverage:

http://lab.llvm.org:8080/coverage/coverage-reports/llvm/coverage/Users/buildslave/jenkins/workspace/clang-stage2-coverage-R/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp.html

Build NewMask using iterators

• Add comments
• dyn_cast -> cast

Fix outdated comment

In D66537#1643292, @lebedev.ri wrote:

I'm not sure how (if at all) to review patches like this?
Just accept that none of the existing tests change and the diff seems sane?

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.

rebase and regenerate test

Restore check for qvfcmpeq

They are indeed new nodes that end up being added to the worklist when their user is processed.