[PATCH] D87651: [AArch64][SVE] Implement extractelement of i1 vectors.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

I just don't buy this argument.  You're saying that we must force all target's to perform custom lowering for promotable illegal operations on i1 vectors because somebody in the future might try to "fix" LegalizeDAG when they should implement custom lowering for extend/truncate operations.  This is the situation Target/AArch64 is in and we never considered "fixing" LegalizeDAG so I don't see why others wouldn't just follow our example.

I've already pointed out:

  setOperationAction(ISD::FADD, MVT::v4f16, Promote);
  AddPromotedToType(ISD::FADD, MVT::v4f16, MVT::v4f32);

that exists today and is happy to emit ISD::FP_EXTEND.  We mirrored this to support i1 based int->fp operations, although perhaps that's a case of two wrongs not making a right :)

Sure we can teach LegalizeVectorOps LegalizeDAG's PromoteNode abilities but that feels like wasted effort considering it just works. My understand of the two legalisers is that LegalizeVectorOps might emit scalar operations that require further legalisation, which is a situation that doesn't apply here considering it's vector in vector out.

So ultimately I don't know what to do here.  There's nothing functionally wrong with the patch (other than a couple of minor suggestions) so I guess I'll begrudgingly accept the patch but I just think you're causing artificial restrictions that others have not felt the need to apply and thus hope you'll reconsider.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3323
     EVT VecVT = InVec.getValueType();
+    // computeKnownBits not yet impemented for scalable vectors.
+    if (VecVT.isScalableVector())
----------------
typo "implemented"


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9280-9289
+    auto Count = VT.getVectorElementCount();
+    if (Count.getKnownMinValue() == 2)
+      ScalarVT = MVT::i64;
+    else if (Count.getKnownMinValue() == 4)
+      ScalarVT = MVT::i32;
+    else if (Count.getKnownMinValue() == 8)
+      ScalarVT = MVT::i16;
----------------
You can use getPromotedVTForPredicate here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9292
+    SDValue Extend =
+        DAG.getNode(ISD::SIGN_EXTEND, DL, VectorVT, Op.getOperand(0));
+    MVT ExtractTy = ScalarVT == MVT::i64 ? MVT::i64 : MVT::i32;
----------------
Can ANY_EXTEND be used here?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9296
+                                  Extend, Op.getOperand(1));
+    return DAG.getSExtOrTrunc(Extract, DL, Op.getValueType());
+  }
----------------
As above can this be getAnyExtOrTrunc?


================
Comment at: llvm/test/CodeGen/AArch64/sve-extract-element.ll:490
+; CHECK-NEXT:    fmov w8, s0
+; CHECK-NEXT:    and w0, w8, #0x1
+; CHECK-NEXT:    ret
----------------
Not a problem for this patch but I'm surprised to see the `and` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87651