Ping, rebase

That is indeed the step I was missing. Thanks everybody for getting me up to speed.

In D68195#1757817, @lebedev.ri wrote:

In D68195#1757811, @deadalnix wrote:

Alright, this is the first time I have to commit something with the new github process, and I have to admit, I do not know how to do it - or if I can do it at all. Running git llvm push ask me for a username and password, and my old one do not work. What's the next step for me here?

Did you ever pushed anything to github before, to any other repository?
Did you clone https://github.com/llvm/llvm-project/ via HTTPS or SSH? (should be former!)
It is likely asking for your github login and password.

You may *really* want to follow https://help.github.com/en/github/authenticating-to-github/creating-a-personal-access-token-for-the-command-line to create credentials with which you can *only* commit, but do nothing else

Alright, this is the first time I have to commit something with the new github process, and I have to admit, I do not know how to do it - or if I can do it at all. Running git llvm push ask me for a username and password, and my old one do not work. What's the next step for me here?

Address some of the comments

I'm a bit late to the party, but this is nothing short of amazing. Congratulations!

Also while I'm at it, first sorry for taking so long to review, I missed that patch. Second, if I understand properly, getting node to be processed in the right order actually can collapse arbitrary long addition chains, but it's not clear how to get there, there is a bit of a wack a mole problem that start poping up every time.

Can you please explain a bit more what this is doing? This seems interesting, but I'm not quite sure the pattern is correct.

O => Op

It's indeed not clear if what you have is the result of the legalizer. In this case just proceed.

Add a few comments

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