Page MenuHomePhabricator

[GISel][CombinerHelper] Add concat_vectors(build_vector, build_vector) => build_vector
ClosedPublic

Authored by qcolombet on Oct 16 2019, 2:28 PM.

Details

Summary

Teach the combiner helper how to flatten concat_vectors of build_vectors
into a build_vector.

Add this combine as part of AArch64 pre-legalizer combiner.

Diff Detail

Event Timeline

qcolombet created this revision.Oct 16 2019, 2:28 PM
arsenm added a subscriber: arsenm.Oct 16 2019, 2:36 PM
arsenm added inline comments.
include/llvm/CodeGen/GlobalISel/CombinerHelper.h
108 ↗(On Diff #225308)

ArrayRef?

lib/CodeGen/GlobalISel/CombinerHelper.cpp
92–94 ↗(On Diff #225308)

Should there be a legality check here somewhere? A concat_vectors of build_vector (or possibly build_vector_trunc) is probably the preferred form of sub-32-bit element vectors, but not for 32 or larger.

104–105 ↗(On Diff #225308)

I would expect this to be an assert

test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-concat-vectors.mir
25–27 ↗(On Diff #225308)

Can you add some tests with pointer elements?

qcolombet marked 4 inline comments as done.Oct 16 2019, 2:47 PM
qcolombet added inline comments.
include/llvm/CodeGen/GlobalISel/CombinerHelper.h
108 ↗(On Diff #225308)

Sure!

lib/CodeGen/GlobalISel/CombinerHelper.cpp
92–94 ↗(On Diff #225308)

I think the goal of the helper is just to provide the combine functionality. Whether or not the combine should be applied is left to the caller.

What do you think?

104–105 ↗(On Diff #225308)

I would have expected this too, but decided against it to match the style of the other match functions.
Given the (arguably strange) use of the try methods in tryCombine, an assert does not seem desirable.

Happy to change it though :).

test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-concat-vectors.mir
25–27 ↗(On Diff #225308)

Sure!

qcolombet updated this revision to Diff 225310.Oct 16 2019, 2:55 PM
  • Use ArrayRef instead of const SmallVectorImpl&
  • Add tests with pointer types.
qcolombet marked 2 inline comments as done.Oct 16 2019, 2:55 PM
arsenm added inline comments.Oct 16 2019, 2:59 PM
lib/CodeGen/GlobalISel/CombinerHelper.cpp
92–94 ↗(On Diff #225308)

That makes sense

104–105 ↗(On Diff #225308)

I replaced similar checks in the LegalizationArtifactCombiner in r367376

qcolombet marked an inline comment as done.Oct 16 2019, 3:29 PM
qcolombet added inline comments.
lib/CodeGen/GlobalISel/CombinerHelper.cpp
104–105 ↗(On Diff #225308)

Love it!
I'll fix that one in that PR and the other in another commit.

qcolombet updated this revision to Diff 225321.Oct 16 2019, 3:32 PM
  • Replace check with an assertion
  • Update comments with pre-conditions
qcolombet marked an inline comment as done.Oct 16 2019, 3:33 PM
arsenm accepted this revision.Oct 16 2019, 3:44 PM

LGTM with nit

lib/CodeGen/GlobalISel/CombinerHelper.cpp
113 ↗(On Diff #225321)

I don't think this is needed since the verifier will catch it

126 ↗(On Diff #225321)

I don't think this is needed since the verifier will catch it

139–148 ↗(On Diff #225321)

Pull out repeated getType calls into a variable?

This revision is now accepted and ready to land.Oct 16 2019, 3:44 PM
qcolombet updated this revision to Diff 225328.Oct 16 2019, 4:47 PM
qcolombet marked 2 inline comments as done.
  • Remove asserts already covered by the verifier
  • Put .getType calls into a variable.
qcolombet marked an inline comment as done.Oct 16 2019, 4:47 PM
arsenm accepted this revision.Oct 16 2019, 5:15 PM

LGTM

This revision was automatically updated to reflect the committed changes.