Page MenuHomePhabricator

[GlobalISel] Better verification of G_UNMERGE_VALUES
Needs ReviewPublic

Authored by foad on Oct 5 2021, 4:14 AM.

Details

Summary

Verify three cases of G_UNMERGE_VALUES separately:

  1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).
  2. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).
  3. Splitting a scalar into smaller scalars (the converse of G_MERGE_VALUES).

Diff Detail

Event Timeline

foad created this revision.Oct 5 2021, 4:14 AM
foad requested review of this revision.Oct 5 2021, 4:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2021, 4:14 AM
foad added a comment.EditedOct 5 2021, 4:18 AM
  1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).

Is there any appetite for using a new (G_UNCONCAT_VECTORS???) opcode for this case?

Unfortunately there are a couple of tests that fail the verification I implemented for this case:

Failed Tests (2):

LLVM :: CodeGen/AArch64/GlobalISel/combine-unmerge.mir
LLVM :: CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

by building MIR like this: %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0:_(<2 x s32>)

Should this be allowed?

  1. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

  1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).

Is there any appetite for using a new (G_UNCONCAT_VECTORS???) opcode for this case?

Unfortunately there are a couple of tests that fail the verification I implemented for this case:

Failed Tests (2):

LLVM :: CodeGen/AArch64/GlobalISel/combine-unmerge.mir
LLVM :: CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

by building MIR like this: %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0:_(<2 x s32>)

Should this be allowed?

I don't like unmerge being able to do that. Seems better to handle that with a bitcast first.

  1. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

As for splitting unmerge into separate opcodes, I don't have a particularly strong preference so I'm leaning towards keeping the status quo for now.

  1. Splitting a vector into subvectors (the converse of G_CONCAT_VECTORS).

Is there any appetite for using a new (G_UNCONCAT_VECTORS???) opcode for this case?

Unfortunately there are a couple of tests that fail the verification I implemented for this case:

Failed Tests (2):

LLVM :: CodeGen/AArch64/GlobalISel/combine-unmerge.mir
LLVM :: CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir

by building MIR like this: %1:_(<2 x s16>), %2:_(<2 x s16>) = G_UNMERGE_VALUES %0:_(<2 x s32>)

Should this be allowed?

It probably shouldn't be

  1. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

foad added a comment.Oct 19 2021, 6:01 AM
  1. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

You mean, splitting a vector into vectors vs splitting a vector into scalars? Then why do we have both G_BUILD_VECTOR and G_CONCAT_VECTORS? The asymmetry annoys me!

foad updated this revision to Diff 380671.Oct 19 2021, 6:16 AM

Adjust MIR test cases to void G_UNMERGE_VALUES from vector to vector
with different element type.

  1. Splitting a vector into its elements (the converse of G_BUILD_VECTOR).

Is there any appetite for using a new G_SPLIT_VECTOR opcode for this case?

I don't see a reason to have a scalar and vector version of the same thing.

You mean, splitting a vector into vectors vs splitting a vector into scalars? Then why do we have both G_BUILD_VECTOR and G_CONCAT_VECTORS? The asymmetry annoys me!

I'm not against splitting up unmerge into separate opcodes for symmetry. The original motivation of splitting G_MERGE was that the legalizer rules were very complex to deal with all the different cases that the instruction could handle.