[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 a comment.

A gentle ping.

I addressed all comments except a few that I'd like to discuss additionally.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2575
+  unsigned PolicyOp = getVecPolicyOpIdx(*UI, MCID);
+  return UI->getConstantOperandVal(PolicyOp) & RISCVII::TAIL_AGNOSTIC;
+}
----------------
asi-sc wrote:
> 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?
This is still unresolved. A whitelist approach worries me. What should be the rule to distinguish good and bad candidates for the whitelist? @reames, could you please share your thoughts?

Is the testing comprehensive enough to follow my suggestion: split the patch, commit parts separately and check the impact on tests?


================
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)
----------------
asi-sc wrote:
> 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.
@reames, do you insist on simplifying all generic code to process only one-use nodes? Original issue #55615 requires multiple uses support, so eventually it must be introduced anyway.  Currently, use-walk is disabled in a way that we can enable it back by removing two lines on the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130895