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

[Anton Sidorenko via Phabricator via llvm-commits]

asi-sc added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2572
+    return true;
+  if (!RISCVII::hasVecPolicyOp(MCID.TSFlags))
+    return true;
----------------
reames wrote:
> 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.  
Thanks for noticing. I changed to return false and added FIXME comment.


================
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())) {
----------------
reames wrote:
> getVLOperand
We cannot use getVLOperands here (and getVLOpIdx a few lines below) as Node is RISCVISD.  At the same time we know that it's VMV/VFMV, so we can be pretty sure in the positioning of operands.


================
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.
----------------
reames wrote:
> Extract a helper.  e.g. isVLLessThan(VL, VL)
> 
> Since this runs after selection, I think you need to special case RISCV::VLMaxSentinel.  
Actually, VLMaxSentinel was handled implicitly (as it is -1ULL and we did zero extension). But I agree we must check it explicitly and do not rely on the exact value of it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130895