This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] combine G_TRUNC with G_MERGE_VALUES
ClosedPublic

Authored by gargaroff on Mar 10 2020, 5:51 AM.

Details

Summary

Truncating the result of a merge means that most likely we could have done without merge in the first place and just used the input merge inputs directly. This can be done in three cases:

  1. If the truncation result is smaller than the merge source, we can use the source in the trunc directly
  2. If the sizes are the same, we can replace the register or use a copy
  3. If the truncation size is a multiple of the merge source size, we can build a smaller merge

This gets rid of most of the larger, hard-to-legalize merges.

Diff Detail

Event Timeline

gargaroff created this revision.Mar 10 2020, 5:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2020, 5:52 AM

Needs dedicated test for this specific combine

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
236

You can initialize this to NumSrc size to avoid the push_back in the loop

Needs dedicated test for this specific combine

I didn't find any dedicated artifact combiner tests where I could add mine. Could you point me in the right direction?

gargaroff updated this revision to Diff 249395.Mar 10 2020, 8:28 AM

Initialize SrcRegs to exact size

gargaroff marked an inline comment as done.Mar 10 2020, 8:29 AM

Needs dedicated test for this specific combine

I didn't find any dedicated artifact combiner tests where I could add mine. Could you point me in the right direction?

I put some of these in test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-*

paquette added inline comments.Mar 10 2020, 2:26 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
203

It would be nice to just early return here and avoid a level of indentation.

if (!DstTy.isScalar() || !MergeSrcTy.isScalar())
    return false;
242–244

Can we put braces around this else?

gargaroff updated this revision to Diff 249571.Mar 11 2020, 2:56 AM

Address review comments. Add combiner test to AMDGPU

gargaroff marked 2 inline comments as done.Mar 11 2020, 2:57 AM

Btw, this pre-merge check keeps failing because the loop-counter variable i is not upper-case. Is that something that I should fix? Because I was under the impression that we use lower-case for loop-counters.

arsenm added inline comments.Mar 11 2020, 9:12 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-trunc.mir
2 ↗(On Diff #249571)

Can probably remove -global-isel-abort=0

98 ↗(On Diff #249571)

Should add some cases with pointers to catch the isScalar checks

gargaroff updated this revision to Diff 249909.Mar 12 2020, 5:20 AM

Add additional pointer tests. Remove -global-isel-abort flag from test

gargaroff marked 3 inline comments as done.Mar 12 2020, 6:17 AM
gargaroff added inline comments.
llvm/test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-trunc.mir
98 ↗(On Diff #249571)

I already had a test for merging pointers to a scalar. Since G_TRUNC cannot work on pointers there is no point in having a merge from scalars to pointers, as this would require a G_PTRTOINT cast, which would not trigger this combine rule in the first place.

I therefore simply added the three cases (trunc size < merge source size, >, ==) with pointer inputs and check that they do not get combined. I hope that's enough.

gargaroff marked an inline comment as done.Mar 13 2020, 2:38 AM
arsenm accepted this revision.Mar 13 2020, 9:08 AM
This revision is now accepted and ready to land.Mar 13 2020, 9:08 AM