Page MenuHomePhabricator

[globalisel][legalizer] Combine G_TRUNC+G_MERGE_VALUES in artifact combiner
AcceptedPublic

Authored by dsanders on May 23 2019, 11:52 AM.

Details

Summary

This has a fairly good chance of killing off significant amounts of dead
code when narrowScalar() is used to legalize certain instructions since
it removes vestigial uses of the upper component of a G_MERGE_VALUES that
may not contribute to the final value. For example, a sequence of s128
operations narrowScalar'd to s64 that ends in a truncation to s64 can
discard all the operations for the upper s64. As they are no longer
kept alive by the use of the lower s64.

Event Timeline

dsanders created this revision.May 23 2019, 11:52 AM
arsenm added inline comments.May 23 2019, 12:01 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
430

No else after return

431

Aren't these both required to be scalars anyway? If a vector is involved you have to use G_CONCAT_VECTOR or G_BUILD_VECTOR

dsanders marked an inline comment as done.May 23 2019, 12:08 PM
dsanders added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
431

Good point. I'd forgotten we had split vectors out

dsanders updated this revision to Diff 201048.May 23 2019, 1:03 PM

G_MERGE_VALUES always has scalars
Fixed a else after return

arsenm accepted this revision.May 23 2019, 7:21 PM

LGTM with nit

llvm/unittests/CodeGen/GlobalISel/LegalizerArtifactCombinerTest.cpp
88

EXPECT_EQ(0, .size())

127

EXPECT_EQ(0, .size())

This revision is now accepted and ready to land.May 23 2019, 7:21 PM

Patch alone looks good.
I'm just coming from D61787, hopefully it shouldn't be to complicated to make them both work together.
Also targets does not have to define any legalization rules for this combine to work which is great.

In D61787 I mentioned that it might be easier for artifact combiner if this was handled with adding narrow scalar rule for G_TRUNC(G_UNMERGE+COPY) and allowing combiner to finish with combining G_UNMERGE/G_MERGE.
What do you think is better in the sense that we also want to allow legalization of chained artifacts?

Now we have G_SEXT/G_TRUNC combine and G_TRUNC/G_UNMERGE_VALUES, which one should happen first in a sequence like this?

%2:_(s128) = G_MERGE_VALUES %0:_(s64), %1:_(s64) 
%3:_(s64) = G_TRUNC %2(s128)
%4:_(s128) = G_SEXT %3:_(s64)
  . . .   = G_UNMERGE_VALUES %4:_(s128)
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-unmerge-values.mir
321–325

Uncombined G_UNMERGE/G_MERGE pair.

dsanders updated this revision to Diff 214501.Aug 9 2019, 8:01 PM

Rebased to master

dsanders updated this revision to Diff 214710.Aug 12 2019, 2:29 PM

Rebase
Note: The addition of s128 G_LOAD/G_STORE to AArch64's made test_inserts_[123] from test/CodeGen/AArch64/GlobalISel/legalize-inserts.mir unusable so they are only tested in LegalizerArtifactCombinerTest.cpp now

This looks like it's done already on master?