[PATCH] D130895: [RISCV] Make VL choosing for a splat-like VMV based on its users

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:1814
+    // Try to shrink VL for a splat-like move.
+    if (Opcode == RISCVISD::VMV_V_X_VL || Opcode == RISCVISD::VFMV_V_F_VL) {
+      SDNode *UpdatedNode = tryShrinkVLForVMV(Node);
----------------
We will need to extend this for splats of constants eventually.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2508
+  uint64_t MaxVLConstant = 0;
+  for (SDNode *Node : Range) {
+    SDValue VL = getVLOperand(Node, TII);
----------------
This loop is redundant under one-use restriction.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2548
+
+  const RISCVVPseudosTable::PseudoInfo *RVV =
+      RISCVVPseudosTable::getPseudoInfo(UI->getMachineOpcode());
----------------
Pull this block out as a helper called: canReadyPastVL.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2569
+  const MCInstrDesc &MCID = TII.get(UI->getMachineOpcode());
+  if (!RISCVII::hasMergeOp(MCID.TSFlags) ||
+      getMergeOpIdx(*UI, MCID) != UI.getOperandNo())
----------------
It's not clear to me why merge ops have anything to do with the legality here.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2572
+    return true;
+  if (!RISCVII::hasVecPolicyOp(MCID.TSFlags))
+    return true;
----------------
Policy is implicit for instructions without policy, it is safer to return false here.

Otherwise, look at the tied operand handling in RISCVInsertVSETVLI.cpp for what we'd need to factor out and reuse.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2575
+  unsigned PolicyOp = getVecPolicyOpIdx(*UI, MCID);
+  return UI->getConstantOperandVal(PolicyOp) & RISCVII::TAIL_AGNOSTIC;
+}
----------------
Macro comment: The legality here has me nervous.  Might be good to start with a whitelist approach.  We can come back and loosen it, but starting with a known good set would make the change less risky.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2588
+    return Node;
+  for (SDNode::use_iterator UI = Node->use_begin(), UE = Node->use_end();
+       UI != UE; ++UI)
----------------
This loop is redundant under one-use restriction.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2593
+
+  SDValue VL = getCommonVL(Node->uses(), TII);
+  if (!VL)
----------------
This call should become a single retrieval of VL from the single user under the one-use restriction.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2596
+    return Node;
+  SDValue OldVL = Node->getOperand(Node->getNumOperands() - 1);
+  if (auto *ConstantOldVL = dyn_cast<ConstantSDNode>(OldVL.getNode())) {
----------------
getVLOperand


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2598
+  if (auto *ConstantOldVL = dyn_cast<ConstantSDNode>(OldVL.getNode())) {
+    // OldVL is a constant. Check that the found VL value is smaller than the
+    // old one.
----------------
Extract a helper.  e.g. isVLLessThan(VL, VL)

Since this runs after selection, I think you need to special case RISCV::VLMaxSentinel.  


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2612
+  SmallVector<SDValue, 3> Ops(Node->op_begin(), Node->op_end());
+  Ops[Node->getNumOperands() - 1] = VL;
+  return CurDAG->UpdateNodeOperands(Node, Ops);
----------------
getVLOpIdx


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130895