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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
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];
+      })
----------------
aemerson wrote:
> 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.
> 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D53629