Page MenuHomePhabricator

GlobalISel: Artifact combine unmerge of unmerge
ClosedPublic

Authored by arsenm on Aug 24 2020, 6:29 AM.

Details

Summary

Unmerges have the same fundamental problem as G_TRUNC, and G_TRUNC
could be implemented in terms of G_UNMERGE_VALUES. Reducing the number
of elements in unmerge results ends up producing the original unmerge
type profile, so the artifact combiner needs to eliminate the
intermediate illegal registers. This avoids infinite looping in the
legalizer in a future change.

Assuming an unmerge has each result unmerged the same way, this ends
up producing a new unmerge of the source for every definition. I'm not
sure if the artifact combiner should either insert temporary merges
here and erase the original merge, or if the combiner should look at
uses from defs rather than defs from uses for unmerges.

In a few cases this regresses from using 16-bit shifts for 8-bit
values to using 32-bit shifts, but I think these can be legalized
later (the other legalization rules don't try very hard to use 16-bit
shifts either).

Diff Detail

Event Timeline

arsenm created this revision.Aug 24 2020, 6:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 6:29 AM
arsenm requested review of this revision.Aug 24 2020, 6:29 AM
foad added a subscriber: foad.Tue, Aug 25, 6:27 AM
foad added inline comments.Tue, Aug 25, 6:29 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
512 ↗(On Diff #287371)

Unrelated?

arsenm added inline comments.Tue, Aug 25, 6:47 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
512 ↗(On Diff #287371)

I can drop this. It's kind of unrelated but based on test diffs I noticed

arsenm updated this revision to Diff 288317.Thu, Aug 27, 6:55 AM

Remove comment

foad added a comment.Thu, Aug 27, 7:30 AM

Wrong patch?

arsenm updated this revision to Diff 288327.Thu, Aug 27, 7:32 AM

Correct patch

arsenm planned changes to this revision.Thu, Aug 27, 5:39 PM
arsenm updated this revision to Diff 288495.Thu, Aug 27, 5:47 PM

Fix broken rebase

aemerson added inline comments.Mon, Aug 31, 10:33 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
548

I'm not sure what point this comment block is trying to get across? What do you mean "equivalent unmerge to copy back..."

// %0:_(<4 x s16>) = G_FOO
// %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0
// %3:_(s16), %4:_(s16) = G_UNMERGE_VALUES %1
//
// %3:_(s16), %4:_(s16), %5:_(s16), %6:_(s16) = G_UNMERGE_VALUES %0

For this case, the new unmerge is what this comment is referring to?

569

Is there a test that shows this case?

arsenm added inline comments.Mon, Aug 31, 10:42 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
548

If you were to try to implement FewerElements for a G_UNMERGE_VALUES, you would end up producing the same G_UNMERGE_VALUES. This isn't produced here

569

test_unmerge_values_s16_from_v3s16_from_v6s16 produces multiple identical unmerges

aemerson accepted this revision.Mon, Aug 31, 4:14 PM
aemerson added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
548

Ok got it.

This revision is now accepted and ready to land.Mon, Aug 31, 4:14 PM