[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: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);
----------------
reames wrote:
> We will need to extend this for splats of constants eventually.  
Aren't splats already processed in RISCVDAGToDAGISel::PreprocessISelDAG ? Honestly, I thought that ISD:SPLAT_VECTOR here is a legacy that we can remove.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2575
+  unsigned PolicyOp = getVecPolicyOpIdx(*UI, MCID);
+  return UI->getConstantOperandVal(PolicyOp) & RISCVII::TAIL_AGNOSTIC;
+}
----------------
reames wrote:
> 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.  
One more idea is that we can forbid changing of VL for the instruction if its result is used as a merge operand. And add support for merge operands in the next patch as a separate feature.
Which option do you want me to follow?


================
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)
----------------
reames wrote:
> This loop is redundant under one-use restriction.
Isn't one-use restriction a temporary thing? I've added it in a way that we can easily drop this restriction and try use-walk (see `if` above). Is it a real issue that the code obtaining new VL is more generic than it's required for one-use approach? If you think we might eventually end up with one-use approach, then I agree we must completely remove use-walk support from this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130895