[PATCH] D130895: [RISCV] Make VL choosing for a splat-like VMV based on its users
Anton Sidorenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 21 09:17:14 PDT 2022
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;
+}
----------------
asi-sc wrote:
> 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.
OK, I'll add only load instructions, otherwise the list is too big.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130895/new/
https://reviews.llvm.org/D130895
More information about the llvm-commits
mailing list