On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Hongtao Liu via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > Hi:
> > For inline asm, there could be an operand like (not (mem:)), it's
> > not a valid operand for normal memory constraint.
> > Bootstrap is ok, regression test is ok for make check
> > RUNTESTFLAGS="--target_board='unix{-m32,}'"
> >
> > gcc/ChangeLog
> > PR target/97540
> > * ira.c: (ira_setup_alts): Extract memory from operand only
> > for special memory constraint.
> > * recog.c (asm_operand_ok): Ditto.
> > * lra-constraints.c (process_alt_operands): MEM_P is
> > required for normal memory constraint.
> >
> > gcc/testsuite/ChangeLog
> > * gcc.target/i386/pr97540.c: New test.
>
> Sorry to stick my oar in, but I think we should reconsider the
> bcst_mem_operand approach. It seems like these patches (and the
> previous one) are fighting against the principle that operands
> cannot be arbitrary expressions.
>
> This kind of thing was attempted long ago (even before my time!)
> for SIGN_EXTEND on MIPS. It ended up causing more problems than
> it solved and in the end it had to be taken out. I'm worried that
> we might end up going through the same cycle again.
>
> Also, this LRA code is extremely performance-sensitive in terms
> of compile time: it's often at the top or near the top of the profile.
> So adding calls to new functions like extract_mem_from_operand for
> a fairly niche case probably isn't a good trade-off.
>
> I think we should instead find a nice(?) syntax for generating separate
> patterns for the two bcst_vector_operand alternatives from a single
> .md pattern. That would fit the existing model much more closely.
>
We have define_subst for RTL template transformations, but it's not
suitable for this case(too many define_subst templates need to
be added, and it doesn't show any advantage compared to adding
separate bcst patterns.). I don't find other workable existing syntax for it.
So suppose I should revert my former 2 patches and add separate bcst patterns.
> (To be clear, I'm not saying the existing model is perfect.
> I just think a change along these lines is more fundamental
> than it might look, and would need changes outside the register
> allocators to work reliably.)
>
> FWIW, in:
>
> (define_insn "*<plusminus_insn><mode>3"
> [(set (match_operand:VI_AVX2 0 "register_operand" "=x,v")
> (plusminus:VI_AVX2
> (match_operand:VI_AVX2 1 "bcst_vector_operand" "<comm>0,v")
> (match_operand:VI_AVX2 2 "bcst_vector_operand" "xBm,vmBr")))]
>
> we can assume that any bcst_mem_operand will be first. Allowing it
> as operand 2 (as the constraints do) creates non-canonical RTL.
> So this at least is one case in which I think the bcst_mem_operand
> version has to be a separate .md construct.
>
> Sorry for not noticing or speaking up earlier. I realise it's
> extremely unhelpful to get this kind of comment after you've done
> so much work. :-(
>
> Thanks,
> Richard
--
BR,
Hongtao
