This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes
ClosedPublic

Authored by aemerson on Oct 23 2018, 6:10 PM.

Details

Summary

[GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes.

This patch restricts the capability of G_MERGE_VALUES, and uses the new G_BUILD_VECTOR and G_CONCAT_VECTOR opcodes instead in the appropriate places.

The MIRBuilder is modified to generate G_CONCAT when trying to create MERGE_VALUES of vectors.

This patch also includes AArch64 support for selecting G_BUILD_VECTOR of 4xs32 and 2xs64 vectors.

Diff Detail

Repository
rL LLVM

Event Timeline

aemerson created this revision.Oct 23 2018, 6:10 PM
dsanders added inline comments.Oct 25 2018, 10:30 AM
lib/Target/AArch64/AArch64LegalizerInfo.cpp
320–386

It can be in a follow-up patch but we should simplify this now that we've eliminated a lot of the power these ops had

400–403

This can be in a follow-up patch if you prefer but I don't think these cases should be unsupported. That's really for things that are meaningless or impossible for anyone to legalize. I think we ought to legalize it even if we fewerElements all the way down to scalars.

Ideally, we'd have something along the lines of:

.unsupportedIf([=](const LegalityQuery &Query) {
      // NumOperands doesn't exist right now. We should add it as it's important for variable_ops
      // legalization
      return !Query.Types[0].isVector() || !Query.Types[1].isScalar() ||
             Query.NumOperands != VecTy.getNumElements() + 1;
    })
.legalFor({{v4s32, s32}, {v2s64, s64}})
.clampNumElements(0, s32, v4s32, v4s32)
.clampNumElements(0, s64, v2s64, v2s64)
.maxNumElements(0, 1)
// It's not entirely obvious what we should do with overly large scalars.
// Here I've assumed we keep the SelectionDAG
// semantics of BUILD_VECTOR and allow an implicit truncation
.legalIf([=](const LegalityQuery &Query) {
      return Query.Types[0].getScalarSizeInBits() < Query.Types[1].getSizeInBits();
    })
// At this point we only have to deal with {v4s32, <s32}, {v2s64, <s64}
// I'd suggest adding this one to the library as .minScalarSameAs(1, 0) or similar
.widenScalarIf([=](const LegalityQuery &Query) {
      return Query.Types[0].getScalarSizeInBits() > Query.Types[1].getSizeInBits();
    },
    [=](const LegalityQuery &Query) {
      return std::make_pair(1, Query.Types[0].getElementType());
    } )
404

You don't need to change this unless you also change the unsupportedIf() because that blocks out the incorrect cases but just to mention it:

.legalFor({{v4s32, s32}, {v2s64, s64}})

would be more accurate for this

aemerson added inline comments.Oct 26 2018, 7:11 AM
lib/Target/AArch64/AArch64LegalizerInfo.cpp
320–386

Sure.

400–403

I'm leaning towards disallowing the implicit truncation for G_BUILD_VECTOR operands. Reason being that I don't think it's that useful, but I'm open to arguments for allowing it. If we disallow that, the rule is redundant as that case will be illegal coming into the legalizer.

Thinking more long term, is there any reason we need to have explicitly unsupported cases? Either they are illegal and should be caught by the verifier/never produced, or in cases where we have targets that have implemented only GISel and thus cannot fall back, there is no way to recover from a legalization failure. It's essentially the same as llvm_unreachable(). I guess it might be useful as way to run the legalizer as a verifier pass to check nothing unexpected happened.

404

Sure.

aemerson updated this revision to Diff 171524.Oct 29 2018, 9:38 AM

Updated AArch64 legalization. I've removed the unsupported case since we're actually making it valid.

volkan added inline comments.Nov 6 2018, 3:59 PM
test/CodeGen/AArch64/GlobalISel/legalize-nonpowerof2eltsvec.mir
42

This test fails because G_EXTRACT_VECTOR_ELT for v3s64 is not legal. Could you fix that?

dsanders added inline comments.Nov 6 2018, 5:53 PM
lib/Target/AArch64/AArch64LegalizerInfo.cpp
400–403

Thinking more long term, is there any reason we need to have explicitly unsupported cases? Either they are illegal and should be caught by the
verifier/never produced, or in cases where we have targets that have implemented only GISel and thus cannot fall back, there is no way to recover
from a legalization failure. It's essentially the same as llvm_unreachable(). I guess it might be useful as way to run the legalizer as a verifier pass to
check nothing unexpected happened.

It could be an asserts-only thing. It's easier to debug if we catch a bad input or legalization step the moment it happens.

Hi Amara,

I see a few buildMerge calls in LegalizerHelper. Could you check them to make sure we pick the correct opcode depending on the the destination type?

volkan added inline comments.Nov 26 2018, 2:52 PM
include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
155

Could you handle G_BUILD_VECTOR as well? We are unable to legalize the test below because of the missing combine.

---
name:            test_unmerge
body:             |
  bb.1:
    liveins: $w0
    %0:_(s32) = COPY $w0
    %1:_(<2 x s32>) = G_BUILD_VECTOR %0(s32), %0(s32)
    %2:_(s32), %3:_(s32) = G_UNMERGE_VALUES %1(<2 x s32>)
    $w0 = COPY %2(s32)
...

Hi Volkan,

We are unable to legalize the test below because of the missing combine.

This sentence jumped at me. Are you saying we have mandatory combines at this point for legalization to work?
Although that wouldn't surprise me, I would like we have a clear list of what they are (if that's not already the case.)

Cheers,
-Quentin

Hi Volkan,

We are unable to legalize the test below because of the missing combine.

This sentence jumped at me. Are you saying we have mandatory combines at this point for legalization to work?
Although that wouldn't surprise me, I would like we have a clear list of what they are (if that's not already the case.)

Cheers,
-Quentin

Hi Quentin,

It's not required for legalization, the target should handle that operation without depending on the artifact combiner.
I meant we should handle G_BUILD_VECTOR to avoid regressions as we are able to legalize the same function with G_MERGE_VALUES.

Volkan

aemerson marked an inline comment as done.Nov 28 2018, 2:57 PM
aemerson added inline comments.
include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
155

Yep, working on this and the other issue.

I meant we should handle G_BUILD_VECTOR to avoid regressions as we are able to legalize the same function with G_MERGE_VALUES.

Got it!

aemerson marked an inline comment as done.Nov 28 2018, 5:10 PM
aemerson added inline comments.
test/CodeGen/AArch64/GlobalISel/legalize-nonpowerof2eltsvec.mir
42

This test doesn't seem to do what it says on the tin. It actually never tested any legalizing, commit r299929 introduced it but as far as I can tell, the G_UNMERGE is deleted due to the legalizercombiner, and G_MERGE here is specified as legal by AArch64 so the legalizer never did anything.

I propose deleting this test as it doesn't seem to achieve anything at the moment.

volkan added inline comments.Nov 29 2018, 3:01 PM
test/CodeGen/AArch64/GlobalISel/legalize-nonpowerof2eltsvec.mir
42

I added this test to make sure we don't hit the assertion when legalizing non-power-of-2 types. I think it is okay to remove it as there are already tests with non-power-of-2 types.

aemerson updated this revision to Diff 176226.Nov 30 2018, 4:28 PM

Addressed comments, deleted the non power of 2 test.

aemerson updated this revision to Diff 176508.Dec 3 2018, 4:36 PM

Updated to incorporate Volkan's patches to change buildMerge calls to explicitly check for types before choosing the right build method. Also added test for and fixed artifact combiner for G_BUILD_VECTOR.

volkan accepted this revision.Dec 10 2018, 9:48 AM

Thanks Amara, LGTM.

This revision is now accepted and ready to land.Dec 10 2018, 9:48 AM
aemerson closed this revision.Dec 10 2018, 10:52 AM

Thanks, committed in r348788.