This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Use vslidedown for undef sub-sequences in generic build_vector
ClosedPublic

Authored by reames on May 2 2023, 8:37 AM.

Details

Summary

This is a follow up to D149263 which extends the generic vslide1down handling to use vslidedown (without the one) for undef elements, and in particular for undef sub-sequences. This both removes the domain crossing, and for undef subsequences results in fewer instructions over all.

A question for reviewers. This could also be written as a set of DAG combines and achieve the same effect. I'd actually written it that way originally, but given a) it didn't catch anything beyond the buildvec case and b) preference expressed in previous reviews, I switched over. Which do we prefer here?

Another question - mostly for a possible follow-on. The codegen this produces for a build_vector which ends with a undef sub-sequence is a series of vslide1downs + a one vslidedown at the end. An alternate result would be to change VL, and directly insert into the desired positions. The tradeoff is between a vsetvli toggle (which would be largely unremovable) and an extra vslidedown. I'm leaning towards keeping this codegen, does that seem like the right outcome?

Diff Detail

Event Timeline

reames created this revision.May 2 2023, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 8:37 AM
reames requested review of this revision.May 2 2023, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 8:37 AM
luke added a comment.May 3 2023, 2:45 AM

A question for reviewers. This could also be written as a set of DAG combines and achieve the same effect. I'd actually written it that way originally, but given a) it didn't catch anything beyond the buildvec case and b) preference expressed in previous reviews, I switched over. Which do we prefer here?

I also wrote a DAG combine for merging vslide1downs of undefs to vslidedown locally, and can confirm that it didn't catch anything else.

luke accepted this revision.May 3 2023, 4:54 AM

The gist of this LGTM

Another question - mostly for a possible follow-on. The codegen this produces for a build_vector which ends with a undef sub-sequence is a series of vslide1downs + a one vslidedown at the end. An alternate result would be to change VL, and directly insert into the desired positions. The tradeoff is between a vsetvli toggle (which would be largely unremovable) and an extra vslidedown. I'm leaning towards keeping this codegen, does that seem like the right outcome?

Does it depend if a vsetvli toggle is cheaper than a vslidedown that touches n lanes? Either way I'm happy with the current approach.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
3241

Could we reuse getVSlidedown here?

3251

Nit, we can remove this assignment

This revision is now accepted and ready to land.May 3 2023, 4:54 AM
This revision was landed with ongoing or failed builds.May 3 2023, 7:54 AM
This revision was automatically updated to reflect the committed changes.