From: Wilco Dijkstra <Wilco.Dijkstra@arm.com>
To: Richard Earnshaw <Richard.Earnshaw@arm.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Cc: Richard Sandiford <Richard.Sandiford@arm.com>
Subject: Re: [PATCH] AArch64: Fix strict-align cpymem/setmem [PR103100]
Date: Wed, 20 Sep 2023 17:08:06 +0000 [thread overview]
Message-ID: <PAWPR08MB8982EEDE0DDBF09C94127D8983F9A@PAWPR08MB8982.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <9d14aa1a-91fb-cc6a-cc75-fb6c4447d7a2@arm.com>
Hi Richard,
> * config/aarch64/aarch64.md (cpymemdi): Remove pattern condition.
> Shouldn't this be a separate patch? It's not immediately obvious that this is a necessary part of this change.
You mean this?
@@ -1627,7 +1627,7 @@ (define_expand "cpymemdi"
(match_operand:BLK 1 "memory_operand")
(match_operand:DI 2 "general_operand")
(match_operand:DI 3 "immediate_operand")]
- "!STRICT_ALIGNMENT || TARGET_MOPS"
+ ""
Yes that's necessary since that is the bug.
> + unsigned align = INTVAL (operands[3]);
>
>This should read the value with UINTVAL. Given the useful range of the alignment, it should be OK that we're not using unsigned HWI.
I'll fix that.
> + if (!CONST_INT_P (operands[2]) || (STRICT_ALIGNMENT && align < 16))
> return aarch64_expand_cpymem_mops (operands);
>
> So what about align=4 and copying, for example, 8 or 12 bytes; wouldn't we want a sequence of LDR/STR in that case? Doesn't this fall back to MOPS too eagerly?
The goal was to fix the issue in way that is both obvious and can be easily backported.
Further improvements can be made to handle other alignments, but it is
slightly tricky (eg. align == 4 won't emit LDP/STP directly using current code
and thus would need additional work to generalize the LDP path).
>> + unsigned max_mops_size = aarch64_mops_memcpy_size_threshold;
>
>I find this name slightly confusing. Surely it's min_mops_size (since above that we want to use MOPS rather than inlined loads/stores). But why not just use aarch64_mops_memcpy_size_threshold directly in the one place it's used?
The reason is that in a follow-on patch I check aarch64_mops_memcpy_size_threshold
too, so for now this acts as a shortcut for the ridiculously long name.
> Are there any additional tests for this?
There are existing tests that check the expansion which fail if you completely
block expansions with STRICT_ALIGNMENT.
Cheers,
Wilco
next prev parent reply other threads:[~2023-09-20 17:08 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-20 13:50 Wilco Dijkstra
2023-09-20 15:52 ` Richard Earnshaw (lists)
2023-09-20 17:08 ` Wilco Dijkstra [this message]
2023-09-21 14:24 ` [PATCH v2] " Wilco Dijkstra
2023-10-16 12:48 ` Wilco Dijkstra
2023-11-06 12:09 ` Wilco Dijkstra
2023-11-29 18:09 ` Richard Sandiford
2023-11-30 12:03 ` Richard Earnshaw
2024-06-27 15:00 ` [BACKPORT] " Wilco Dijkstra
2024-06-27 16:40 ` Richard Sandiford
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=PAWPR08MB8982EEDE0DDBF09C94127D8983F9A@PAWPR08MB8982.eurprd08.prod.outlook.com \
--to=wilco.dijkstra@arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=Richard.Sandiford@arm.com \
--cc=gcc-patches@gcc.gnu.org \
/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).