[PATCH] D23464: [PR28367][Reassociation] Avoid iterator invalidation when negating value.

Tue Aug 16 13:34:40 PDT 2016

mcrosier added a subscriber: resistor.
mcrosier added a comment.

In https://reviews.llvm.org/D23464#517105, @efriedma wrote:

> I'm not really happy with this patch... you're basically disabling the CSE optimization for what seems like the most important case.

I don't disagree, but I didn't find this to really matter in my testing (see my comments from a few minutes ago).

> That said, I'm not sure the CSE optimization is actually necessary given the current state of reassociate.  The "look for an existing negation" code was added in r92373 to fix test/Transforms/Reassociate/basictest.ll `@test12`... and that test apparently still passes with this patch.  Maybe it would make sense to just delete the whole loop to look for an existing negate?

I tried that as well and I'm not opposed to this solution.  However, that did cause 4 Reassociation lit tests to regress and it resulted in a bit more code gen changes (i.e., still no changes to SPEC, but I see about 10 changes in the llvm-test-suite).

> Another way to fix the iterator invalidation problem would be to revert r258830, I think.

My gut tells me r258830 only exposed the problem and that it would still exist after a revert (but much less likely to happen).  IIRC, @aditya_nandakumar and @resistor also indicated this commit was necessary to address a performance critical issue they were seeing.

> FYI, this is basically my first time reading the reassociate code, so I might be missing something.

No problem; I appreciate the review.

https://reviews.llvm.org/D23464