[PATCH] D65614: [Reassociate] Stop linearizing all associative expression trees w/o profitability

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D65614#1612409 <https://reviews.llvm.org/D65614#1612409>, @spatel wrote:

> I like the idea of limiting this pass. It annoys me that -reassociate can pseudo-randomly change the order of associative+commutative operands, and those transforms don't play by instcombine's operand complexity canonicalization rules (see getComplexity() in InstCombine). In other words, this pass can (hopefully only benignly) fight with instcombine.


I'm indeed not opposed to preventing this pass from doing NOP work, but still,
i don't think the reasoning/patch description should say anything about linearization/register pressure.
If that is a problem, then you will have the exact same problem if you start with such bad IR before this transform.

In other words, while you can put a band-aid on it by "arbitrarily crippling" passes,
it may only stop degradation  of IR, but nothing will be improving the already-bad IR.
So i'd guess this really should be solved more head-on.

> But I agree with Roman's analysis of the test diffs - we need to preserve the cases where reassociation/factorization eliminates instructions. InstCombine doesn't handle that generally, and we don't want to shift that burden to InstCombine because that pass is already expensive.




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

https://reviews.llvm.org/D65614