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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2575
+  unsigned PolicyOp = getVecPolicyOpIdx(*UI, MCID);
+  return UI->getConstantOperandVal(PolicyOp) & RISCVII::TAIL_AGNOSTIC;
+}
----------------
asi-sc wrote:
> 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?
How do you propose to split the patch?  

One way would be to introduce a narrow whitelist, land that version, and then relax it with dedicated testing and review.  Was that what you had in mind?  Or something else?


================
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:
> 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.
Yes, I do. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130895