[PATCH] D153848: [RISCV] Apply promotion for f16 vector ops when only have zvfhmin.

[Michael Maitland via Phabricator via llvm-commits]

michaelmaitland added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:169
 
+  void PromoteReduction(SDNode *Node, SmallVectorImpl<SDValue> &Results);
+
----------------
nit: can you add a docstring?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:574
+              .isFloatingPoint() &&
+          NewVecVT.isVector() &&
+          NewVecVT.getVectorElementType().isFloatingPoint())
----------------
`NewVecVT` name implies that it is already a vector type. Do you need to check it here? Would it be better to assert right after you assign to `NewVecVT` that it isVector?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:575
+          NewVecVT.isVector() &&
+          NewVecVT.getVectorElementType().isFloatingPoint())
+        Operands[j] =
----------------
`NewVecVT.getVectorElementType().isFloatingPoint()` -> `NewScalarVT.isFloatingPoint()`?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:575
+          NewVecVT.isVector() &&
+          NewVecVT.getVectorElementType().isFloatingPoint())
+        Operands[j] =
----------------
michaelmaitland wrote:
> `NewVecVT.getVectorElementType().isFloatingPoint()` -> `NewScalarVT.isFloatingPoint()`?
> 
Does it matter if `NewVecTy` isVector? Does it matter if `NewVecVT.getVectorElementType().isFloatingPoint()`? If all we're worried about is promoting a f16 operand to a f32 operand, I think we may be able to just check if the operand isFP, and bitcast otherwise.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:580
+        Operands[j] =
+            DAG.getNode(ISD::BITCAST, DL, NewVecVT, Node->getOperand(j));
+    else if (Node->getOperand(j).getValueType() == ScalarVT &&
----------------
Why BITCAST instead of ANY_EXTEND, but for the scalar below we ANY_EXTEND?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:581
+            DAG.getNode(ISD::BITCAST, DL, NewVecVT, Node->getOperand(j));
+    else if (Node->getOperand(j).getValueType() == ScalarVT &&
+             ISD::getVPExplicitVectorLengthIdx(Node->getOpcode()) != j)
----------------
I think that the `start_value` is always the 0th argument. I tested this by instrumenting the code with `assert(j ==0)` when this case was true.

Maybe the 0th argument (this one that triggers this case) can be handled outside the loop to simplify this nested if branch, and the loop can start at 1?




================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:662
+    // Do not promote the mask operand of a VP OP.
+    bool SkipPromote = (ISD::isVPOpcode(Node->getOpcode()) &&
+                        ISD::getVPMaskIdx(Node->getOpcode()) == j);
----------------
I think you can drop surrounding `()` here.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:824
+
+    // TODO: support more vp ops.
+    static const unsigned ZvfhminPromoteVPOps[] = {
----------------
are there any more you plan on adding to this patch?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:936
+
+        // custom split nxv32f16.
+        if (VT == MVT::nxv32f16) {
----------------
`custom` -> `Custom`.

Can you also clarify in comment that the reason we need to split is because promotion would lead to LMUL16 without splitting, but we don't have LMUL16?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153848