Page MenuHomePhabricator

[VectorCombine] Run load/extract scalarization after scalarizing store.
AbandonedPublic

Authored by fhahn on Apr 12 2021, 6:38 AM.

Details

Summary

Remove the insertelement instruction (via D98240) can enable further
load/extract scalarization opportunities. Try it again.

Diff Detail

Event Timeline

fhahn created this revision.Apr 12 2021, 6:38 AM
fhahn requested review of this revision.Apr 12 2021, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 6:38 AM
fhahn updated this revision to Diff 345392.May 14 2021, 4:02 AM

Rebase and update to actually pass the first user.

fhahn updated this revision to Diff 347701.May 25 2021, 9:09 AM

rebase and ping :) All pending patches should be committed now.

Some questions were raised about D98240, so we should deal with those before adding this.

fhahn updated this revision to Diff 365954.Aug 12 2021, 3:42 AM

Rebase and ping :)

Some questions were raised about D98240, so we should deal with those before adding this.

I think those questions should be addressed by now and the issues raised by Alive2 should also be ironed out now.

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

That's a fair point. But I think the same probably holds true fo somer other combines in VectorCombine. So should the whole pass just use a worklist?

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

That's a fair point. But I think the same probably holds true fo somer other combines in VectorCombine. So should the whole pass just use a worklist?

That was my comment precisely.

fhahn added a comment.Aug 17 2021, 1:33 PM

I do not like the general direction VectorCombine seems to be going in.
Doing only a single pass and hoping that the second pass wouldn't do anything clearly leads to problems if (!Load->user_empty()) { is solving.
I would think we'd really want an instcombine-lite approach here, with a worklist..

That's a fair point. But I think the same probably holds true fo somer other combines in VectorCombine. So should the whole pass just use a worklist?

That was my comment precisely.

@spatel did you by any chance already think/consider this for VectorCombine? Or do you have any concerns?

@spatel did you by any chance already think/consider this for VectorCombine? Or do you have any concerns?

No concerns. This has come up before (we were walking the instruction bottom-up at some point instead of top-down), but we just didn't have enough motivation with the existing transforms to justify implementing a worklist. If you want to try it, that would be great!

fhahn abandoned this revision.Tue, Sep 21, 7:00 AM

@spatel did you by any chance already think/consider this for VectorCombine? Or do you have any concerns?

No concerns. This has come up before (we were walking the instruction bottom-up at some point instead of top-down), but we just didn't have enough motivation with the existing transforms to justify implementing a worklist. If you want to try it, that would be great!

I put up a patch: D110171

I'll abandon the patch here for now.