Page MenuHomePhabricator

[Passes] Move AggressiveInstCombine after InstCombine
AcceptedPublic

Authored by anton-afanasyev on Thu, Nov 4, 3:47 AM.

Details

Summary

Swap AIC and IC neighbouring in pipeline. This looks more natural and even
almost has no effect for now (three slightly touched tests of test-suite). Also
this could be the first step towards merging AIC (or its part) to -O2 pipeline.

After several changes in AIC (like D108091, D108201, D107766, D109515, D109236)
there've been observed several regressions (like PR52078, PR52253, PR52289)
that were fixed in different passes (see D111330, D112721) by extending their
functionality, but these regressions were exposed since changed AIC prevents IC
from making some of early optimizations.

This is common problem and it should be fixed by just moving AIC after IC
which looks more logically by itself: make aggressive instruction combining
only after failed ordinary one.

Fixes PR52289

Diff Detail

Event Timeline

anton-afanasyev created this revision.Thu, Nov 4, 3:47 AM
anton-afanasyev requested review of this revision.Thu, Nov 4, 3:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 4, 3:47 AM
lebedev.ri edited the summary of this revision. (Show Details)Thu, Nov 4, 3:50 AM

Isn't it just a symptom that said passes need to be enhanced?
They will still not handle the 'bad' patterns even if AIC no longer produces them beforehand.

spatel added a comment.Thu, Nov 4, 5:12 AM

Isn't it just a symptom that said passes need to be enhanced?
They will still not handle the 'bad' patterns even if AIC no longer produces them beforehand.

I agree (and I put an instcombine fold candidate in PR52289), but I also think this is a good change.
AIC isn't generally expecting non-canonical IR, so it could be missing transforms if it encounters weird IR (for example, a commutative binop with constant as operand 0).
Is there anything in the commit logs or patch reviews that explains why AIC was put before instcombine in the pipeline?

Isn't it just a symptom that said passes need to be enhanced?
They will still not handle the 'bad' patterns even if AIC no longer produces them beforehand.

That's definitely a symptom, sure. I've waited some time to gather unveiled issues and going to wait more till all of them seem reported and fixed. But I still believe this change should be ever committed.

Is there anything in the commit logs or patch reviews that explains why AIC was put before instcombine in the pipeline?

I've passed through the long story of its review (D38313) and haven't seen special attention to this point. Initially AIC was implemented as a part of InstCombine, running _before_ its iterations of pattern-matching, but it was splitted out to separate pass in a process of review (https://reviews.llvm.org/D38313#906667).

spatel added a comment.Thu, Nov 4, 7:23 AM

Is there anything in the commit logs or patch reviews that explains why AIC was put before instcombine in the pipeline?

I've passed through the long story of its review (D38313) and haven't seen special attention to this point. Initially AIC was implemented as a part of InstCombine, running _before_ its iterations of pattern-matching, but it was splitted out to separate pass in a process of review (https://reviews.llvm.org/D38313#906667).

Thanks for digging that up. I don't see any reason that it was placed that way either, so LGTM. Please wait to see if @lebedev.ri agrees though.

We should update/file bugs for the regressions that were uncovered by the previous AIC change, so they are independent of -O3/AIC and we don't lose those. Do you know if there others -- apart from the ones listed in the summary: PR52078 (fixed), PR52253 (in progress), PR52289 (updated/forked) ?

We should update/file bugs for the regressions that were uncovered by the previous AIC change, so they are independent of -O3/AIC and we don't lose those. Do you know if there others -- apart from the ones listed in the summary: PR52078 (fixed), PR52253 (in progress), PR52289 (updated/forked) ?

That's full list for now.

Thanks for digging that up. I don't see any reason that it was placed that way either, so LGTM. Please wait to see if @lebedev.ri agrees though.

Ping @lebedev.ri .

any more comments? @lebedev.ri @spatel

There was some older differential that proposed doing this,
IIRC there was disscussion there, but i'm failing to find it now.
Bu general recollection is that the order is intentional
to avoid pattern disturbance by instcombine before AIC has a chance.

Bu general recollection is that the order is intentional
to avoid pattern disturbance by instcombine before AIC has a chance.

Hmm, it would be great to see an example. I've seen the reverse cases for now: disturbance by AIC before IC.

Btw, the only impact seen by this patch is almost zero, the test suite shows three size-reduced tests and no regressions:

$ ~/llvm/test-suite/utils/compare.py -a --filter-short -m size..text result_base.json vs result_exp.json
 ...
 test-suite...abench/jpeg/jpeg-6a/cjpeg.test   167826.00  167810.00  -0.0%
 test-suite...nsumer-jpeg/consumer-jpeg.test   167186.00  167170.00  -0.0%
 test-suite...lications/sqlite3/sqlite3.test   496674.00  496546.00  -0.0%
spatel accepted this revision.Mon, Nov 22, 12:06 PM

any more comments? @lebedev.ri @spatel

I'll make my earlier acceptance official here in Phab.
I can't find any earlier reviews/comments to justify the ordering, there are no regression tests to support it, and we have evidence that this (new/proposed) ordering helps at least one case (and we still have a bug tracking the root cause).

This revision is now accepted and ready to land.Mon, Nov 22, 12:06 PM

any more comments? @lebedev.ri @spatel

I'll make my earlier acceptance official here in Phab.
I can't find any earlier reviews/comments to justify the ordering, there are no regression tests to support it, and we have evidence that this (new/proposed) ordering helps at least one case (and we still have a bug tracking the root cause).

@lebedev.ri Need your final approval :)

lebedev.ri resigned from this revision.Wed, Dec 1, 10:37 AM