[PATCH] D53629: [GlobalISel] Restrict G_MERGE_VALUES capability and replace with new opcodes

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:320-386
     // FIXME: This rule is horrible, but specifies the same as what we had
     // before with the particularly strange definitions removed (e.g.
     // s8 = G_MERGE_VALUES s32, s32).
     // Part of the complexity comes from these ops being extremely flexible. For
     // example, you can build/decompose vectors with it, concatenate vectors,
     // etc. and in addition to this you can also bitcast with it at the same
     // time. We've been considering breaking it up into multiple ops to make it
----------------
dsanders wrote:
> 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
Sure.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:400-403
+      .unsupportedIf([=](const LegalityQuery &Query) {
+        const LLT &VecTy = Query.Types[0];
+        return VecTy.getElementType() != Query.Types[1];
+      })
----------------
dsanders wrote:
> 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());
>       } )
> 
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.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:404
+      })
+      .legalForCartesianProduct({v4s32, v2s64}, {s32, s64});
+
----------------
dsanders wrote:
> 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
Sure.


Repository:
  rL LLVM

https://reviews.llvm.org/D53629