[PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hongtao Liu]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

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.

--
BR,
Hongtao

[-- Attachment #2: 0002-Don-t-extract-memory-from-operand-for-normal-memory-.patch --]
[-- Type: application/x-patch, Size: 3358 bytes --]

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

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.

(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

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hongtao Liu]

On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
<richard.sandiford@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

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Hongtao Liu <crazylht@gmail.com> writes:
> On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
> <richard.sandiford@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.

Yeah, I think it would need to be new syntax.  I was wondering if it
would help if we had somethine like (strawman suggestion):

	  (one_of 0
	    [(match_operand:VI_AVX2 1 "vector_operand" "...")
	     (vec_duplicate:VI_AVX2
	       (match_operand:<...> 1 "..." "..."))]

where all instances of (one_of N ...) for a given N are required
to have the same number of alternatives.

This could be handled in a similar way to define_subst, with the
one_of being expanded before the main generator routines see it.

But maybe it wouldn't help that much.  E.g. for:

(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")))]

the vec_duplicate version should only really have one alternative.
I guess we could handle that by using a:

  (one_of 0
    [(set_attr "enabled" "*,*")
     (set_attr "enabled" "0,*")])

or some variant of that that uses a derived attribute.  But it feels
a bit clunky…

Without that, I guess the only pattern that would benefit directly is:

(define_insn "avx512dq_mul<mode>3<mask_name>"
  [(set (match_operand:VI8_AVX512VL 0 "register_operand" "=v")
	(mult:VI8_AVX512VL
	  (match_operand:VI8_AVX512VL 1 "bcst_vector_operand" "%v")
	  (match_operand:VI8_AVX512VL 2 "bcst_vector_operand" "vmBr")))]

> So suppose I should revert my former 2 patches and add separate bcst patterns.

Are there going to more patterns that need bcst_vector_operand,
or is the current set complete?

I definitely think we should have a better way of handling this in the
.md files, and I'd be happy to hack something up on the generator side
(given that I'm being the awkward one here).  But I guess the answer to
the question above will decide whether it make things better or not.

FWIW, I think having separate patterns (whether they're produced from
one .md construct or from several) might better optimisation results.
But I guess there's a risk of combinatorial explosion, and the port has
a lot of patterns as it is.

Thanks,
Richard

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hongtao Liu]

On Thu, Oct 29, 2020 at 2:46 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Hongtao Liu <crazylht@gmail.com> writes:
> > On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
> > <richard.sandiford@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.
>
> Yeah, I think it would need to be new syntax.  I was wondering if it
> would help if we had somethine like (strawman suggestion):
>
>           (one_of 0
>             [(match_operand:VI_AVX2 1 "vector_operand" "...")
>              (vec_duplicate:VI_AVX2
>                (match_operand:<...> 1 "..." "..."))]
>
> where all instances of (one_of N ...) for a given N are required
> to have the same number of alternatives.
>
> This could be handled in a similar way to define_subst, with the
> one_of being expanded before the main generator routines see it.
>
> But maybe it wouldn't help that much.  E.g. for:
>
> (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")))]
>
> the vec_duplicate version should only really have one alternative.
It would be guaranteed by it's attribute (set_attr "isa" "noavx,avx"),
since bcst_mem_operand implies avx512f, which of course implies avx,
therefore the first alternative would never be enabled under avx.
> I guess we could handle that by using a:
>
>   (one_of 0
>     [(set_attr "enabled" "*,*")
>      (set_attr "enabled" "0,*")])
>
> or some variant of that that uses a derived attribute.  But it feels
> a bit clunky…
>
> Without that, I guess the only pattern that would benefit directly is:
>
> (define_insn "avx512dq_mul<mode>3<mask_name>"
>   [(set (match_operand:VI8_AVX512VL 0 "register_operand" "=v")
>         (mult:VI8_AVX512VL
>           (match_operand:VI8_AVX512VL 1 "bcst_vector_operand" "%v")
>           (match_operand:VI8_AVX512VL 2 "bcst_vector_operand" "vmBr")))]
>
> > So suppose I should revert my former 2 patches and add separate bcst patterns.
>
> Are there going to more patterns that need bcst_vector_operand,

Almost all AVX512 instructions need corresponding bcst patterns except
for those with 8-bit/16-bit data elements.
And the former patch at [1] is the first step to refactor the existing
bcst patterns. If it works without extra issue(unfortunately not),
I'll convert more patterns to bcst_vector_operand.

> or is the current set complete?
>
> I definitely think we should have a better way of handling this in the
> .md files, and I'd be happy to hack something up on the generator side
> (given that I'm being the awkward one here).  But I guess the answer to
> the question above will decide whether it make things better or not.
>
> FWIW, I think having separate patterns (whether they're produced from
> one .md construct or from several) might better optimisation results.

With proper extending for special memory constraint, they should be equivalent.
At least unit tests under i386 backend would generate embedded
broadcast instruction as before.

> But I guess there's a risk of combinatorial explosion, and the port has
> a lot of patterns as it is.
>
> Thanks,
> Richard


[1] https://gcc.gnu.org/pipermail/gcc-patches/2020-October/555948.html
-- 
BR,
Hongtao

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hongtao Liu]

On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
<richard.sandiford@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.
>

Could you provide the thread link for the issue of SIGN_EXTEND on
MIPS, then I can take a look to see if it's exactly the same issue as
mine.



-- 
BR,
Hongtao

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Jakub Jelinek]

On Tue, Oct 27, 2020 at 11:13:21AM +0000, Richard Sandiford via Gcc-patches wrote:
> 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.

Many operands already are fairly complex expressions, so it is unclear how
this changes that.
And LRA etc. already handles SUBREGs of MEM which is kind of similar to
this.

> 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.

It can be just an inline function that looks through just the target
selected rtxes rather than arbitrary ones (derived from *.md properties or
something).

> 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.

That would result in thousands of new patterns, I'm not sure it is a good
idea.  Pretty much all AVX512* instructions allow those.

	Jakub

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Hongtao Liu via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Thu, Oct 29, 2020 at 2:46 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Hongtao Liu <crazylht@gmail.com> writes:
>> > On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
>> > <richard.sandiford@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.
>>
>> Yeah, I think it would need to be new syntax.  I was wondering if it
>> would help if we had somethine like (strawman suggestion):
>>
>>           (one_of 0
>>             [(match_operand:VI_AVX2 1 "vector_operand" "...")
>>              (vec_duplicate:VI_AVX2
>>                (match_operand:<...> 1 "..." "..."))]
>>
>> where all instances of (one_of N ...) for a given N are required
>> to have the same number of alternatives.
>>
>> This could be handled in a similar way to define_subst, with the
>> one_of being expanded before the main generator routines see it.
>>
>> But maybe it wouldn't help that much.  E.g. for:
>>
>> (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")))]
>>
>> the vec_duplicate version should only really have one alternative.
> It would be guaranteed by it's attribute (set_attr "isa" "noavx,avx"),
> since bcst_mem_operand implies avx512f, which of course implies avx,
> therefore the first alternative would never be enabled under avx.

Ah, OK.

>> I guess we could handle that by using a:
>>
>>   (one_of 0
>>     [(set_attr "enabled" "*,*")
>>      (set_attr "enabled" "0,*")])
>>
>> or some variant of that that uses a derived attribute.  But it feels
>> a bit clunky…
>>
>> Without that, I guess the only pattern that would benefit directly is:
>>
>> (define_insn "avx512dq_mul<mode>3<mask_name>"
>>   [(set (match_operand:VI8_AVX512VL 0 "register_operand" "=v")
>>         (mult:VI8_AVX512VL
>>           (match_operand:VI8_AVX512VL 1 "bcst_vector_operand" "%v")
>>           (match_operand:VI8_AVX512VL 2 "bcst_vector_operand" "vmBr")))]
>>
>> > So suppose I should revert my former 2 patches and add separate bcst patterns.
>>
>> Are there going to more patterns that need bcst_vector_operand,
>
> Almost all AVX512 instructions need corresponding bcst patterns except
> for those with 8-bit/16-bit data elements.

OK, that changes things.  Sorry, not knowing the architecture,
I wasn't sure how far this was from being complete.

>> or is the current set complete?
>>
>> I definitely think we should have a better way of handling this in the
>> .md files, and I'd be happy to hack something up on the generator side
>> (given that I'm being the awkward one here).  But I guess the answer to
>> the question above will decide whether it make things better or not.
>>
>> FWIW, I think having separate patterns (whether they're produced from
>> one .md construct or from several) might better optimisation results.
>
> With proper extending for special memory constraint, they should be equivalent.
> At least unit tests under i386 backend would generate embedded
> broadcast instruction as before.

I guess my main objection is that we have a special memory constraint
that isn't in fact matching a MEM (at least not directly).  That seems
odd and feels like it's going to come back to bite us.

From an RTL perspective, the MEM in the bcst_memory_operand isn't that
special.  It's really just a normal memory that happens to appear in a
VEC_DUPLICATE.

With the MIPS thing, the SIGN_EXTEND was around a register rather
than a memory, and the register constraints applied to the register
as normal.  In other words, the SIGN_EXTEND was not part of the
constraint: target-independent code removed the SIGN_EXTEND
before matching against the constraints.

I think we could view the bcst_mem_operand case in a similar way.
The constraint process ends up having two steps: an extraction
step (for the REG inside a SIGN_EXTEND for MIPS, for the MEM
inside a VEC_DUPLICATE for AVX512) and the normal matching step on
the result.  This could either happen on a constraint-by-constraint
basis or (perhaps more efficiently) as a separate step before
applying the constraints.  Not sure off-hand which is better,
would need to think more about it.

But I think this extraction process should be described directly in the
.md file somehow.  I've had a few ideas but I'm not happy enough with
any of them yet to post.

Thanks,
Richard

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Hongtao Liu via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
> <richard.sandiford@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.
>>
>
> Could you provide the thread link for the issue of SIGN_EXTEND on
> MIPS, then I can take a look to see if it's exactly the same issue as
> mine.

I couldn't find anything, sorry.  The patch that finally removed
the MIPS handling was:

  https://gcc.gnu.org/pipermail/gcc-patches/2002-October/088178.html

I know there was some discussion about the problems around then,
but some of it might have been private rather than on-list.
I can't remember the details now.

Thanks,
Richard

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Jakub Jelinek <jakub@redhat.com> writes:
> On Tue, Oct 27, 2020 at 11:13:21AM +0000, Richard Sandiford via Gcc-patches wrote:
>> 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.
>
> Many operands already are fairly complex expressions, so it is unclear how
> this changes that.

But the things subject to constraint matching currently have to be
SCRATCHes, SUBREGs, REGs, MEMs or constants.  The address inside
a MEM can be complex, but even that has certain limits (so that LRA
knows what to do with addresses that need reloading).

Matching something like VEC_DUPLICATE in a constraint is new in
that thing being constrained isn't conceptually an object
(only the operand of the VEC_DUPLICATE is).

> And LRA etc. already handles SUBREGs of MEM which is kind of similar to
> this.

Yeah, but SUBREGs of MEMs are a bit of a legacy feature :-)
It would be great to remove them at some point…

>> 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.
>
> It can be just an inline function that looks through just the target
> selected rtxes rather than arbitrary ones (derived from *.md properties or
> something).

Having something in the .md file sounds good.  The more information the
generators have, the more chance they have to do something efficient.

>> 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.
>
> That would result in thousands of new patterns, I'm not sure it is a good
> idea.  Pretty much all AVX512* instructions allow those.

Yeah, I hadn't realised that.

Thanks,
Richard

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hans-Peter Nilsson]

On Thu, 29 Oct 2020, Richard Sandiford via Gcc-patches wrote:
> Hongtao Liu via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > On Tue, Oct 27, 2020 at 7:13 PM Richard Sandiford
> > <richard.sandiford@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.
> >>
> >
> > Could you provide the thread link for the issue of SIGN_EXTEND on
> > MIPS, then I can take a look to see if it's exactly the same issue as
> > mine.
>
> I couldn't find anything, sorry.  The patch that finally removed
> the MIPS handling was:
>
>   https://gcc.gnu.org/pipermail/gcc-patches/2002-October/088178.html
>
> I know there was some discussion about the problems around then,
> but some of it might have been private rather than on-list.
> I can't remember the details now.

To further muddy the waters, I vaguely remember the SH makes (or
made) use of some peculiar feature regarding (optional?)
sign/zero-extend in operands.  Joern and I had an "exchange of
views" where I (IIRC) wanted to remove a supporting middle-end
feature which was in the way of something I wanted to do and
actually didn't help the SH.  (Register allocation silently
swallowing an unary operator in constraints?)  I think he
conceded but the legwork to remove it was never done.

Point being, to make that bell ring, perhaps it helps to look in
mail archives even long before then (maybe before 2000) or at
least a look at the SH port.  Looking there now, maybe it's
related to its arith_reg_operand, that oddly appears to allow
sign_extend but where the body doesn't match it due to an #if 0.
So, all support has likely decayed since then and now be just
dust...

brgds, H-P

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hongtao Liu]

On Fri, Oct 30, 2020 at 1:00 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> I guess my main objection is that we have a special memory constraint
> that isn't in fact matching a MEM (at least not directly).  That seems
> odd and feels like it's going to come back to bite us.
>
> From an RTL perspective, the MEM in the bcst_memory_operand isn't that
> special.  It's really just a normal memory that happens to appear in a
> VEC_DUPLICATE.
>
> With the MIPS thing, the SIGN_EXTEND was around a register rather
> than a memory, and the register constraints applied to the register
> as normal.  In other words, the SIGN_EXTEND was not part of the
> constraint: target-independent code removed the SIGN_EXTEND

Yes, that's because target-independent code can't distinguish whether
SIGN_EXTEND is part of constraints. More specifically, constrain_operands
 will swallow the unary operator when matching constraints. Cut from recog.c
-----
          /* A unary operator may be accepted by the predicate, but it
             is irrelevant for matching constraints.  */
          /* For special_memory_operand, there could be a memory operand inside,
             and it would cause a mismatch for constraint_satisfied_p.  */
          if (UNARY_P (op) && op == extract_mem_from_operand (op))
            op = XEXP (op, 0);
------

But a unary operator with memory would never be accepted by the predicate
unless it's corresponding to special_memory_constraint, because in IRA/LRA,
CT_MEMORY would never be matched when op is false for MEM_P.

> before matching against the constraints.
>
> I think we could view the bcst_mem_operand case in a similar way.
> The constraint process ends up having two steps: an extraction
> step (for the REG inside a SIGN_EXTEND for MIPS, for the MEM
> inside a VEC_DUPLICATE for AVX512) and the normal matching step on
> the result.  This could either happen on a constraint-by-constraint
> basis or (perhaps more efficiently) as a separate step before
> applying the constraints.  Not sure off-hand which is better,
> would need to think more about it.
>
> But I think this extraction process should be described directly in the
> .md file somehow.  I've had a few ideas but I'm not happy enough with
> any of them yet to post.
>
> Thanks,
> Richard



-- 
BR,
Hongtao

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Hongtao Liu <crazylht@gmail.com> writes:
> On Fri, Oct 30, 2020 at 1:00 AM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> I guess my main objection is that we have a special memory constraint
>> that isn't in fact matching a MEM (at least not directly).  That seems
>> odd and feels like it's going to come back to bite us.
>>
>> From an RTL perspective, the MEM in the bcst_memory_operand isn't that
>> special.  It's really just a normal memory that happens to appear in a
>> VEC_DUPLICATE.
>>
>> With the MIPS thing, the SIGN_EXTEND was around a register rather
>> than a memory, and the register constraints applied to the register
>> as normal.  In other words, the SIGN_EXTEND was not part of the
>> constraint: target-independent code removed the SIGN_EXTEND
>
> Yes, that's because target-independent code can't distinguish whether
> SIGN_EXTEND is part of constraints. More specifically, constrain_operands
>  will swallow the unary operator when matching constraints. Cut from recog.c
> -----
>           /* A unary operator may be accepted by the predicate, but it
>              is irrelevant for matching constraints.  */
>           /* For special_memory_operand, there could be a memory operand inside,
>              and it would cause a mismatch for constraint_satisfied_p.  */
>           if (UNARY_P (op) && op == extract_mem_from_operand (op))
>             op = XEXP (op, 0);
> ------

Yeah, this is a vestige from the old code to support MIPS-style
SIGN_EXTEND constraints.  I can't remember why it wasn't removed.
Maybe noone thought about it, or maybe it's what H-P said about SH.

reload also still has code for this, but it's telling that LRA and IRA don't.

> But a unary operator with memory would never be accepted by the predicate
> unless it's corresponding to special_memory_constraint, because in IRA/LRA,
> CT_MEMORY would never be matched when op is false for MEM_P.

I think my point is kind of the opposite though: in both the MIPS and the
AVX512 cases, we effectively want the constrained operand to be different
from the operand matched by the predicate.  So I think the old way of
getting target-independent code to dig down into unary expressions is
conceptually cleaner.  Even better would be to have .md constructs that
say exactly what the constraints should match for a given predicate,
but that's obviously more work (and probably not for GCC 11).

I realise it's a bit odd to be warning on the one hand that the old
approach seemed to have problems while at the same time advocating for
following something like the old approach, but still. :-)

For example, there's no conceptual reason why a target couldn't support
VEC_DUPLICATEs of registers.  And if it supported VEC_DUPLICATEs of
both registers and memory, we'd want to allow registers to be spilled
to memory in-place, without generating a reload.  To do that, the thing
being constrained has to be the operand of the VEC_DUPLICATE rather than
the VEC_DUPLICATE itself.

So for AVX512 I think we should handle this by getting target-independent
code to look into the VEC_DUPLICATE and make “Br” match a normal (scalar)
memory operand whose mode satisfies VALID_BCST_MODE_P.

I'm experimenting with introducing accessors for getting the
“constraint version” of recog_data.operand, recog_data.operand_loc
and recog_data.operand_mode (hopefully low-overhead).  For now they
just look inside UNARY_P, as before.  Too early to say whether it'll
work though…

Thanks,
Richard

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Vladimir Makarov]

On 2020-10-27 2:53 a.m., Hongtao Liu wrote:
> 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.
>
I understand Richard's concerns and actually these concerns were my 
motivations to constraint possible cases for extract_mem_from_operand in 
the original patch introducing the function.

If Richard proposes a better solution we will reconsider the current 
approach and revert the changes if it is necessary.

Meanwhile I am approving this patch.  I hope it will not demotivate 
Richard's attempt to find a better solution.

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On 2020-10-27 2:53 a.m., Hongtao Liu wrote:
>> 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.
>>
> I understand Richard's concerns and actually these concerns were my 
> motivations to constraint possible cases for extract_mem_from_operand in 
> the original patch introducing the function.
>
> If Richard proposes a better solution we will reconsider the current 
> approach and revert the changes if it is necessary.
>
> Meanwhile I am approving this patch.  I hope it will not demotivate 
> Richard's attempt to find a better solution.

OK, that's fine with me.  I might come back to this next stage 1,
depending on how things turn out.

Richard

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Hongtao Liu]

On Tue, Nov 3, 2020 at 9:51 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > On 2020-10-27 2:53 a.m., Hongtao Liu wrote:
> >> 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.
> >>
> > I understand Richard's concerns and actually these concerns were my
> > motivations to constraint possible cases for extract_mem_from_operand in
> > the original patch introducing the function.
> >
> > If Richard proposes a better solution we will reconsider the current
> > approach and revert the changes if it is necessary.
> >
> > Meanwhile I am approving this patch.  I hope it will not demotivate
> > Richard's attempt to find a better solution.
>
> OK, that's fine with me.  I might come back to this next stage 1,
> depending on how things turn out.
>
> Richard

Thanks for all your comments, patch committed.
And I'm not going to add "Br" to more patterns until the final
solution is in place.

-- 
BR,
Hongtao

Re: [PATCH][PR target/97540] Don't extract memory from operand for normal memory constraint.

[Richard Sandiford]

Hongtao Liu <crazylht@gmail.com> writes:
> On Tue, Nov 3, 2020 at 9:51 PM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> > On 2020-10-27 2:53 a.m., Hongtao Liu wrote:
>> >> 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.
>> >>
>> > I understand Richard's concerns and actually these concerns were my
>> > motivations to constraint possible cases for extract_mem_from_operand in
>> > the original patch introducing the function.
>> >
>> > If Richard proposes a better solution we will reconsider the current
>> > approach and revert the changes if it is necessary.
>> >
>> > Meanwhile I am approving this patch.  I hope it will not demotivate
>> > Richard's attempt to find a better solution.
>>
>> OK, that's fine with me.  I might come back to this next stage 1,
>> depending on how things turn out.
>>
>> Richard
>
> Thanks for all your comments, patch committed.
> And I'm not going to add "Br" to more patterns until the final
> solution is in place.

Please don't hold off on my account.  I think any future update is
likely to be mechanical and having more example uses will be helpful.

Thanks,
Richard