Page MenuHomePhabricator

[GISel][ArtifactCombiner] Relax the constraint to combine unmerge with concat_vectors
ClosedPublic

Authored by qcolombet on Mon, Oct 21, 5:21 PM.

Details

Summary

The combine G_UNMERGE_VALUES with G_CONCAT_VECTORS used to only be performed
when the result type of the G_UNMERGE_VALUES was a vector type.
In other words, we were expecting that the G_UNMERGE_VALUES was effectively
the exact opposite of the G_CONCAT_VECTORS.

Lift that constraint by allowing any G_UNMERGE_VALUES to be combined
with any G_CONCAT_VECTORS (as long as the size of the different pieces
that we merge/unmerge match).

Note: The reason why we had this restriction was I believe to avoid G_UNMERGE_VALUES with scalars as definitions and a vector as input. I.e., an equivalent of a sort of G_UNBUILD_VECTOR. However, we don't have such vector-to-scalar specific opcode, so the limitation was artificial (AFAICT) and we actually already produce such patterns! (See added test case that are derived directly from the added integration test).

Diff Detail

Event Timeline

qcolombet created this revision.Mon, Oct 21, 5:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Oct 21, 5:21 PM
arsenm added inline comments.Mon, Oct 21, 7:27 PM
test/CodeGen/AArch64/GlobalISel/legalizer-combiner.mir
91 ↗(On Diff #225979)

Is there a reason to have the ADJCALLSTACK?

qcolombet marked an inline comment as done.Tue, Oct 22, 8:05 AM
qcolombet added inline comments.
test/CodeGen/AArch64/GlobalISel/legalizer-combiner.mir
91 ↗(On Diff #225979)

No, there is not :).
The test physregs below were originally feeding a call.
I'll remove it.

qcolombet updated this revision to Diff 226143.Wed, Oct 23, 8:20 AM
  • Remove ADJUSTxxx operations in the tests
qcolombet marked an inline comment as done.Wed, Oct 23, 8:20 AM
arsenm accepted this revision.Thu, Oct 24, 4:05 PM

LGTM

This revision is now accepted and ready to land.Thu, Oct 24, 4:05 PM
  • Fix a latent bug
  • Update AMDGPU tests
qcolombet marked an inline comment as done.Thu, Oct 24, 10:31 PM

Hi @arsenm ,

I found a latent bug that has now more chances to trigger with the relaxations that this patch brings.
I've updated the patch to fix the bug, but I have now a bunch of "regressions" on AMDGPU test cases because the fix is too conservative.
Is this still okay to land?

\\ The Bug \\

Basically, before we rewrite the merge/unmerge pair, when a conversion operation is present, we need to make sure that the final source and destination types that will end up in the conversion are legit.
I've appended a test case that triggers the bug on the old (i.e., before this patch) code in test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir.
Without my fix, we could assert that we are trying to create conversions from incompatible types (e.g., sext s16 to <2 x s16>).
There is room for improvement, but I think this patch still goes in the right direction.

\\ The Fix \\
The fix I came up with is pretty conservative:
Let us consider:

dstMerge = merge srcMerge
srcUnmerge = convertop dstMerge
dstUnmerge = unmerge srcUnmerge

If dstUnmerge.isVector() != srcMerge.isVector(), I block the transformation. The rationale being that we might end up rewriting the previous sequence in:

dstUnmerge = convertop srcMerge

Cheers,
-Quentin

include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
231 ↗(On Diff #226376)

This checks block some of the combines that were done with the old code.

qcolombet marked an inline comment as done.Thu, Oct 24, 10:31 PM
qcolombet added inline comments.
include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
226 ↗(On Diff #226376)

Second check.

@arsenm ping on the AMDGPU test changes.

@arsenm ping^2 on the AMDGPU test changes.

arsenm added a comment.Mon, Nov 4, 9:16 AM

Are you planning on fixing the regression in the near future? If so splitting the patches this way seems fine

test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir
230 ↗(On Diff #226376)

This is definitely worse since unmerge to s1 should be illegal, although it's possible the rules aren't properly defined here

320 ↗(On Diff #226376)

This is worse because the truncates need to be scalarized

501 ↗(On Diff #226376)

Test name doesn't quite match, I see no s128

Are you planning on fixing the regression in the near future? If so splitting the patches this way seems fine

I plan to fix the regressions, but I cannot commit on a deadline yet. In particular, I don't expect I will have time the next coming few weeks.

The reason I still believe it makes sense to land the patch as is is because the code without this patch will assert on inputs that looks like test_unmerge_values_s128_of_zext_of_concat_vectors (yes the name doesn't make sense, I'll fix that :)). I.e., I would rather generate worse code (like you mentioned) than potentially crashing the compiler.

What do you think?

arsenm accepted this revision.Mon, Nov 4, 11:30 AM

LGTM, I guess a crash is worse than things that can fall back

This revision was automatically updated to reflect the committed changes.