[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: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
----------------
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


================
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];
+      })
----------------
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());
      } )



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


Repository:
  rL LLVM

https://reviews.llvm.org/D53629