[PATCH] D94708: [IR] Introduce llvm.experimental.vector.splice intrinsic

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 3 inline comments as done.
c-rhodes added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1093
       setOperationAction(ISD::SELECT, VT, Custom);
+      setOperationAction(ISD::SETCC, VT, Custom);
       setOperationAction(ISD::SDIV, VT, Custom);
----------------
c-rhodes wrote:
> cameron.mcinally wrote:
> > Is this change required for the splice patch? I wonder if it should be broken out to its own commit.
> > Is this change required for the splice patch? I wonder if it should be broken out to its own commit.
> 
> This fixes a selection error I hit when splicing predicates. The promotion of predicates uses a truncate which gets lowered to a setcc that crashes on operands of type `VT`, e.g.:
> 
> ```      t17: nxv2i64 = and t29, t30
>       t31: nxv2i64 = AArch64ISD::DUP Constant:i64<0>
>     t19: nxv2i1 = setcc t17, t31, setne:ch```
> 
> I can split this into a separate patch.
> > Is this change required for the splice patch? I wonder if it should be broken out to its own commit.
> 
> This fixes a selection error I hit when splicing predicates. The promotion of predicates uses a truncate which gets lowered to a setcc that crashes on operands of type `VT`, e.g.:
> 
> ```      t17: nxv2i64 = and t29, t30
>       t31: nxv2i64 = AArch64ISD::DUP Constant:i64<0>
>     t19: nxv2i1 = setcc t17, t31, setne:ch```
> 
> I can split this into a separate patch.

I tried splitting this out but I couldn't figure out how to test it so I've left it in this patch. What I found was the setcc being generated by the splice occurs after result type legalization, so without the above a setcc returning an `nxv2i1` and taking two `nxv2i64` vectors is considered legal at this point in selection. With the above it falls into `SelectionDAGLegalize::LegalizeOp` which considers the legality based on the operand VTs, so setcc then gets custom lowered to the target-specific predicated variant `AArch64ISD::SETCC_MERGE_ZERO` that can be selected.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1258
+
+    setOperationAction(ISD::VECTOR_SPLICE, MVT::nxv2i1, Promote);
+    AddPromotedToType(ISD::VECTOR_SPLICE, MVT::nxv2i1, MVT::nxv2i64);
----------------
craig.topper wrote:
> Can you use setOperationPromotedToType here?
> Can you use setOperationPromotedToType here?

That's nice, wasn't aware of that thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94708/new/

https://reviews.llvm.org/D94708