This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Combine G_UNMERGE_VALUES with G_TRUNC
ClosedPublic

Authored by arsenm on Apr 4 2020, 1:43 PM.

Details

Summary

G_MERGE_VALUES with different types, but G_UNMERGE_VALUES of a vector
can also be implemented with a bitcast to a scalar, which introduces
the possibility for infinite loops. Try to eliminate an illegal source
register type in the artifact combiner to avoid this from happening.

Avoids infinite looping in the legalizer in a future patch which
allows lowering G_UNMERGE_VALUES of a vector source with a G_BITCAST.

Diff Detail

Event Timeline

arsenm created this revision.Apr 4 2020, 1:43 PM
aemerson added inline comments.Apr 13 2020, 4:36 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
443

How do we make the decision that this the resulting output is "better" than the input? Can we check for input legality or something?

arsenm updated this revision to Diff 257380.Apr 14 2020, 9:31 AM

Fix checking wrong instruction opcode, more tests

arsenm marked an inline comment as done.Apr 14 2020, 9:34 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
443

I don't think anywhere in the artifact combiner is deciding what's really "better". I think what makes most sense is checking if the vector truncate is legal, but that doesn't really work due to the special constraints on G_TRUNC legality. Checking if the unmerge skips cases where it's still beneficial to do the combine, so I'm not sure what else I would do here

arsenm marked an inline comment as done.Apr 14 2020, 9:43 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
443

I've also been wondering if the conventions of the artifact combiner derive from originally missing most legalization actions on merge/unmerge etc. isInstUnsupported only makes sense to me for poorly defined legalization rules that don't try to handle standalone artifacts as legalizable operations

The description says:

Try to eliminate an illegal source register type in the artifact combiner to avoid this from happening.

I don't see this happening in this patch.

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 10:57 AM
arsenm added a comment.May 2 2020, 1:18 PM

The description says:

Try to eliminate an illegal source register type in the artifact combiner to avoid this from happening.

I don't see this happening in this patch.

It eliminates the intermediate illegal vector. In the example in the comment, this is the <4 x s8> vector. It is replaced by doing the unmerge on the original <4 x s32> vector, and inserting scalar G_TRUNCs. Also see my previous question about what "better" really means in the artifact combiner context. Skipping this either or both input operations are already legal blocks this in cases where it still would produce fewer artifacts.

This revision is now accepted and ready to land.May 4 2020, 10:16 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-trunc.mir