From: Richard Sandiford <richard.sandiford@arm.com>
To: Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org>
Cc: "H.J. Lu" <hjl.tools@gmail.com>,
Richard Biener <richard.guenther@gmail.com>,
Richard Earnshaw <rearnsha@arm.com>
Subject: Re: [PATCH v2] Add TARGET_MOVE_WITH_MODE_P
Date: Wed, 09 Mar 2022 18:04:11 +0000 [thread overview]
Message-ID: <mptv8wnc89w.fsf@arm.com> (raw)
In-Reply-To: <CAFiYyc3ms8QgC5u13ZGuBo2ByjQhhSPn3a8MvnuGrobCn0gePw@mail.gmail.com> (Richard Biener via Gcc-patches's message of "Mon, 7 Mar 2022 14:45:40 +0100")
Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> On Wed, Mar 2, 2022 at 10:18 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>> On Wed, Mar 02, 2022 at 09:51:26AM +0100, Richard Biener wrote:
>> > On Tue, Mar 1, 2022 at 11:41 PM H.J. Lu via Gcc-patches
>> > <gcc-patches@gcc.gnu.org> wrote:
>> > >
>> > > Add TARGET_FOLD_MEMCPY_MAX for the maximum number of bytes to fold memcpy.
>> > > The default is
>> > >
>> > > MOVE_MAX * MOVE_RATIO (optimize_function_for_size_p (cfun))
>> > >
>> > > For x86, it is MOVE_MAX to restore the old behavior before
>> >
>> > I know we've discussed this to death in the PR, I just want to repeat here
>> > that the GIMPLE folding expects to generate a single load and a single
>> > store (that is what it does on the GIMPLE level) which is why MOVE_MAX
>> > was chosen originally (it's documented to what a "single instruction" does).
>> > In practice MOVE_MAX does not seem to cover vector register sizes
>> > so Richard pulled MOVE_RATIO which is really intended to cover
>> > the case of using multiple instructions for moving memory (but then I
>> > don't remember whether for the ARM case the single load/store GIMPLE
>> > will be expanded to multiple load/store instructions).
>> >
>> > TARGET_FOLD_MEMCPY_MAX sounds like a stop-gap solution,
>> > being very specific for memcpy folding (we also fold memmove btw).
>> >
>> > There is also MOVE_MAX_PIECES which _might_ be more appropriate
>> > than MOVE_MAX here and still honor the idea of single instructions.
>> > Now neither arm nor aarch64 define this and it defaults to MOVE_MAX,
>> > not MOVE_MAX * MOVE_RATIO.
>> >
>> > So if we need a new hook then that hook should at least get the
>> > 'speed' argument of MOVE_RATIO and it should get a better name.
>> >
>> > I still think that it should be possible to improve the insn check to
>> > avoid use of "disabled" modes, maybe that's also a point to add
>> > a new hook like .move_with_mode_p or so? To quote, we do
>>
>> Here is the v2 patch to add TARGET_MOVE_WITH_MODE_P.
>
> Again I'd like to shine light on MOVE_MAX_PIECES which explicitely
> mentions "a load or store used TO COPY MEMORY" (emphasis mine)
> and whose x86 implementation would already be fine (doing larger moves
> and also not doing too large moves). But appearantly the arm folks
> decided that that's not fit and instead (mis-?)used MOVE_MAX * MOVE_RATIO.
It seems like there are old comments and old documentation that justify
both interpretations, so there are good arguments on both sides. But
with this kind of thing I think we have to infer the meaning of the
macro from the way it's currently used, rather than trusting such old
and possibly out-of-date and contradictory information.
FWIW, I agree that (if we exclude old reload, which we should!) the
only direct uses of MOVE_MAX before the patch were not specific to
integer registers and so MOVE_MAX should include vectors if the
target wants vector modes to be used for general movement.
Even if people disagree that that's the current meaning, I think it's
at least a sensible meaning. It provides information that AFAIK isn't
available otherwise, and it avoids overlap with MAX_FIXED_MODE_SIZE.
So FWIW, I think it'd be reasonable to change non-x86 targets if they
want vector modes to be used for single-insn copies.
Thanks,
Richard
next prev parent reply other threads:[~2022-03-09 18:04 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-01 22:41 [PATCH] Add TARGET_FOLD_MEMCPY_MAX H.J. Lu
2022-03-02 8:51 ` Richard Biener
2022-03-02 21:18 ` [PATCH v2] Add TARGET_MOVE_WITH_MODE_P H.J. Lu
2022-03-07 13:45 ` Richard Biener
2022-03-08 15:43 ` H.J. Lu
2022-03-09 8:25 ` Richard Biener
2022-03-09 18:07 ` H.J. Lu
2022-03-10 8:20 ` Richard Biener
2022-03-09 18:04 ` Richard Sandiford [this message]
2022-03-10 8:06 ` Richard Biener
2022-03-14 15:44 ` Richard Sandiford
2022-03-22 2:50 ` H.J. Lu
2022-03-22 8:20 ` Richard Biener
2022-03-23 13:58 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mptv8wnc89w.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hjl.tools@gmail.com \
--cc=rearnsha@arm.com \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).