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

[Anton Sidorenko via Phabricator via llvm-commits]

asi-sc planned changes to this revision.
asi-sc added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2575
+  unsigned PolicyOp = getVecPolicyOpIdx(*UI, MCID);
+  return UI->getConstantOperandVal(PolicyOp) & RISCVII::TAIL_AGNOSTIC;
+}
----------------
reames wrote:
> 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?
I dumped all opcodes of the vmv uses that reach this point in the code for llvm-test-suite and SPEC2017. Just to remind: at this point we know that vmv has a single use, and this use is a merge operand in a TA instruction. List of the opcodes:
  - PseudoVLUXEI64_V_M1_M1_MASK
  - PseudoVLUXEI64_V_M1_MF2_MASK
  - PseudoVLSE16_V_M1_MASK
  - PseudoVLSE32_V_M1_MASK
  - PseudoVLSE32_V_MF2_MASK
  - PseudoVFMADD_VF64_M1

So, I think we should add basic arithmetic and load instructions to the whitelist. However, it won't be so narrow as  @reames suggests.


================
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:
> 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. 
OK, thanks for your reply. It'll be done in the next patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130895