This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] PromoteIntRes_EXTRACT_SUBVECTOR for scalable vectors (widening).
ClosedPublic

Authored by sdesmalen on Sep 9 2021, 7:24 AM.

Details

Summary

This patch implements legalization of EXTRACT_SUBVECTOR for the case
where the result needs promoting, and the input type requires widening.

Diff Detail

Event Timeline

sdesmalen created this revision.Sep 9 2021, 7:24 AM
sdesmalen requested review of this revision.Sep 9 2021, 7:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2021, 7:24 AM

Do you think it'd be possible to add RVV tests too? vscale x 2 doesn't need widening but vscale x 3 or something would.

sdesmalen updated this revision to Diff 371856.Sep 10 2021, 3:34 AM

Duplicated tests.

Do you think it'd be possible to add RVV tests too? vscale x 2 doesn't need widening but vscale x 3 or something would.

Sure np. I've copied the same tests that I added for AArch64 and re-ran the update-test-checks script.

Do you think it'd be possible to add RVV tests too? vscale x 2 doesn't need widening but vscale x 3 or something would.

Sure np. I've copied the same tests that I added for AArch64 and re-ran the update-test-checks script.

Thanks! But <vscale x 2 x i8> isn't illegal for us so, if I'm not mistaken, it's not hitting this particular promotion path.

Do you think it'd be possible to add RVV tests too? vscale x 2 doesn't need widening but vscale x 3 or something would.

Sure np. I've copied the same tests that I added for AArch64 and re-ran the update-test-checks script.

Thanks! But <vscale x 2 x i8> isn't illegal for us so, if I'm not mistaken, it's not hitting this particular promotion path.

Ah, sorry I misunderstood that!

What result type would require promotion? I'd need an example where the result would need to be promoted and the input widened.

Ah, sorry I misunderstood that!

What result type would require promotion? I'd need an example where the result would need to be promoted and the input widened.

My bad, I should have been clearer. I hope I wasn't too hasty in my suggestion; I was initially thinking of something like <vscale x 3 x i4> from <vscale x 9 x i4> but they both need promoting and widening, and now I've forgotten in which order those take place.

Ah, sorry I misunderstood that!

What result type would require promotion? I'd need an example where the result would need to be promoted and the input widened.

My bad, I should have been clearer. I hope I wasn't too hasty in my suggestion; I was initially thinking of something like <vscale x 3 x i4> from <vscale x 9 x i4> but they both need promoting and widening, and now I've forgotten in which order those take place.

It seems that trying with an example of i4's (e.g. extracting nxv2i4 from nxv12i4), it actually runs into various other failures that would need fixing first, e.g. widening the truncate to i4's of the input vector. The exact example you gave above (where both need widening), actually fails even earlier in SelectionDAGBuilder::getCopyToPartsVector.

I'm not sure I can come up with an example where it does go down this code path though. Do you want me to keep the tests even if they don't exactly test the code in this patch?

frasercrmck accepted this revision.Sep 10 2021, 4:40 AM

It seems that trying with an example of i4's (e.g. extracting nxv2i4 from nxv12i4), it actually runs into various other failures that would need fixing first, e.g. widening the truncate to i4's of the input vector. The exact example you gave above (where both need widening), actually fails even earlier in SelectionDAGBuilder::getCopyToPartsVector.

I'm not sure I can come up with an example where it does go down this code path though. Do you want me to keep the tests even if they don't exactly test the code in this patch?

Ah yes I see, in the former the truncate is generated during argument lowering. That's a shame.

Then no, I don't think I need these tests because widening the EXTRACT_SUBVECTOR operand is trivial.

Sorry for wasting your time, @sdesmalen. I think this patch LGTM.

This revision is now accepted and ready to land.Sep 10 2021, 4:40 AM

It seems that trying with an example of i4's (e.g. extracting nxv2i4 from nxv12i4), it actually runs into various other failures that would need fixing first, e.g. widening the truncate to i4's of the input vector. The exact example you gave above (where both need widening), actually fails even earlier in SelectionDAGBuilder::getCopyToPartsVector.

I'm not sure I can come up with an example where it does go down this code path though. Do you want me to keep the tests even if they don't exactly test the code in this patch?

Ah yes I see, in the former the truncate is generated during argument lowering. That's a shame.

Then no, I don't think I need these tests because widening the EXTRACT_SUBVECTOR operand is trivial.

Sorry for wasting your time, @sdesmalen. I think this patch LGTM.

No worries, thanks for having a look! I'll remove the tests before committing.

This revision was landed with ongoing or failed builds.Sep 10 2021, 5:30 AM
This revision was automatically updated to reflect the committed changes.