Note that there's an instcombine version of this patch attached there. This reassociate patch took about 10x more effort, so I hope this is the preferred direction. :)

For reasons I still don't completely understand, reassociate does this kind of transform sometimes, but misses everything in my motivating cases.

This patch on its own is gluing an independent cleanup chunk to the end of the existing RewriteExprTree() loop. But if it's approved, I think we can build on it and do something stronger to better order the full expression tree like D40049. That might be an alternative to the proposal to add a separate reassociation pass like D41574.

As far as I can see this is another case of reusing existing Instructions/Values, while changing the actual value that the Instruction produce, right?

Take a look at the debug-info fixes I made here for a similar problem in RewriteExprTree: https://reviews.llvm.org/D45975
I suspect that you may need to discard debug-info in a similar way as in D45975, somewhere inside your new function swapOperandsToMatchBinops.

What are your plans for deeper associate-operand trees, such as in the tests I've suggested at D41574? Will you support such cases?

This patch is independent of D41574 from what I see. Ie, that patch makes no difference on any of these tests. Could it be enhanced to catch these?

If we want to do something like D41574 (provide stronger sorting of the expression tree based on opcode/operand) here in the existing -reassociate, then an addition to ReassociateExpression() like this could be used:

That creates some of the factoring folds that we want, but it won't reduce as far as shown in the other patch. I'm not sure if that's a limitation of this pass or if I just botched the code (and this is untested, so may not be correct).

As far as I can see this is another case of reusing existing Instructions/Values, while changing the actual value that the Instruction produce, right?

Take a look at the debug-info fixes I made here for a similar problem in RewriteExprTree: https://reviews.llvm.org/D45975
I suspect that you may need to discard debug-info in a similar way as in D45975, somewhere inside your new function swapOperandsToMatchBinops.

Thanks! You're correct that we're recycling instructions here (not sure why we don't just create new instructions with a Builder?). The funny thing about all the examples here is that after we swap operands in swapOperandsToMatchBinops(), we end up going through the main reassociation loop again and make more changes which triggers your code D45975, so I already see 'undef' in the right places.

Nevertheless, it's a small change to extract and call that code, so let me do that to be safe.

As far as I can see this is another case of reusing existing Instructions/Values, while changing the actual value that the Instruction produce, right?

Take a look at the debug-info fixes I made here for a similar problem in RewriteExprTree: https://reviews.llvm.org/D45975
I suspect that you may need to discard debug-info in a similar way as in D45975, somewhere inside your new function swapOperandsToMatchBinops.

Thanks! You're correct that we're recycling instructions here (not sure why we don't just create new instructions with a Builder?). The funny thing about all the examples here is that after we swap operands in swapOperandsToMatchBinops(), we end up going through the main reassociation loop again and make more changes which triggers your code D45975, so I already see 'undef' in the right places.

Nevertheless, it's a small change to extract and call that code, so let me do that to be safe.

Hmm, is only test/Transforms/Reassociate/matching-binops.ll regenerated here?
I'm wondering why D46336 changes so many more tests.

Yes, and this is intentional. I don't think we usually want to have IR regression tests that depend on multiple passes. Although in this case - because I've left the actual factoring/distributive optimization out of this patch (at least for now) - it may be worth adding tests under PhaseOrdering to make sure that nothing is interfering with this transform before instcombine has a chance to reduce it.

Hmm, is only test/Transforms/Reassociate/matching-binops.ll regenerated here?
I'm wondering why D46336 changes so many more tests.

Yes, and this is intentional.

Oh right, this is a reassociate pass, not instcombine :)

I don't think we usually want to have IR regression tests that depend on multiple passes. Although in this case - because I've left the actual factoring/distributive optimization out of this patch (at least for now) - it may be worth adding tests under PhaseOrdering to make sure that nothing is interfering with this transform before instcombine has a chance to reduce it.

nit: Verify that we "discard" the dbg.value for variable "a" (metadata !19 in the input, metadata !18 in the output), since we do nto calculate the value %and after the transformation.

@aprantl once told me that is was better to use metadata i32 undef instead of metadata !{} when a dbg.value is "discarded". I think it is out-of-scope for this patch, but maybe the code that picks metadata !2 in this solution should insert an undef value instead (or maybe later passes should handle metadata !{} the same way as if we have an explicit undef value, in case there really is a difference today).

One important take-away from https://reviews.llvm.org/D46336#1090588:
The InstCombine and Reassociate need to be run after each another in a loop (what't the correct term, internal pipeline?) until neither of them produces any more changes.

One important take-away from https://reviews.llvm.org/D46336#1090588:
The InstCombine and Reassociate need to be run after each another in a loop (what't the correct term, internal pipeline?) until neither of them produces any more changes.

If there are no objections, I'll commit this soon. The larger D41574 looks stalled. In the meantime, this is a small patch/improvement that will help limit hyper-extension proposals to instcombine. The patch still applies to trunk cleanly.