[PATCH] D98939: [SelectionDAG][AArch64][SVE] Perform SETCC condition legalization in LegalizeVectorOps

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:775
   case ISD::SETCC:
-    Results.push_back(UnrollVSETCC(Node));
+    ExpandSETCC(Node, Results);
     return;
----------------
Do we need to do something similar for SELECT_CC here too?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1348
+  SDLoc dl(Node);
+  SDValue Tmp1, Tmp2, Tmp3;
+  MVT OpVT = Node->getOperand(0).getSimpleValueType();
----------------
Probably makes sense to declare these in the same place they are assigned?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1352
+
+  if (TLI.getCondCodeAction(CCCode, OpVT) == TargetLowering::Expand) {
+    SDValue Chain;
----------------
Is it worth doing an early return here for the inverse instead? For example,

  if (TLI.getCondCodeAction(CCCode, OpVT) != TargetLowering::Expand) {
    Results.push_back(UnrollVSETCC(Node));
    return;
  }
  ...

That way you can avoid the extra indentation.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1354
+    SDValue Chain;
+    Tmp1 = Node->getOperand(0);
+    Tmp2 = Node->getOperand(1);
----------------
Perhaps worth renaming these to LHS, RHS and CC so it's more readable?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1363
+      // condition code, create a new SETCC node.
+      if (Tmp3.getNode())
+        Tmp1 = DAG.getNode(ISD::SETCC, dl, Node->getValueType(0), Tmp1, Tmp2,
----------------
If it's legal and doesn't need inverting do we even need to reassign Tmp1?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-float-compares.ll:346
+
+define void @fcmp_ueq_v32i8(<16 x half>* %a, <16 x half>* %b, <16 x i16>* %c) #0 {
+; CHECK-LABEL: fcmp_ueq_v32i8:
----------------
I'm not sure where the `i8` comes into the name here - should this be `fcmp_ueq_v16f16` instead? Same comment for other functions below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98939