* Autoinc vs reload and LRA
@ 2019-11-25 20:27 Bernd Schmidt
2019-11-27 1:20 ` Jeff Law
2019-11-28 1:38 ` Segher Boessenkool
0 siblings, 2 replies; 3+ messages in thread
From: Bernd Schmidt @ 2019-11-25 20:27 UTC (permalink / raw)
To: GCC Patches; +Cc: Segher Boessenkool
[-- Attachment #1: Type: text/plain, Size: 819 bytes --]
So I was curious what would happen if I turned on LRA for m68k. It turns
out my autoinc patches from the cc0 patch set expose a bug in how LRA
handles autoincrement. While it copies the logic from reload's
inc_for_reload, it appears to be missing the find_reloads_address code
to ensure an autoinc address is reloaded entirely if it is part of a
jump. LRA can reload just the register inside a POST_INC, which leads to
creating an output reload.
Hence, a new version of the autoinc changes, below. Since reload is
known to work, we allow autoinc in jumps unless targetm.lra_p. One part
of the patch is a fix for the earlier combine patch which was already
checked in, the other part is a new version of the auto-inc-dec patch.
Bootstrapped and tested on the gcc135 machine
(powerpc64le-unknown-linux-gnu). OK?
Bernd
[-- Attachment #2: ainc-v2.diff --]
[-- Type: text/x-patch, Size: 1908 bytes --]
* auto-inc-dec.c (merge_in_block): Allow autoinc in jumps unless
LRA is enabled.
* combine.c (can_combine_p): Disallow autoinc in jumps unless LRA is
disabled.
diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c
index bdb6efab520..1b224cc9777 100644
--- a/gcc/auto-inc-dec.c
+++ b/gcc/auto-inc-dec.c
@@ -1441,10 +1441,9 @@ merge_in_block (int max_reg, basic_block bb)
continue;
}
- /* This continue is deliberate. We do not want the uses of the
- jump put into reg_next_use because it is not considered safe to
- combine a preincrement with a jump. */
- if (JUMP_P (insn))
+ /* Reload should handle auto-inc within a jump correctly, while LRA
+ is known to have issues with autoinc. */
+ if (JUMP_P (insn) && targetm.lra_p ())
continue;
if (dump_file)
diff --git a/gcc/combine.c b/gcc/combine.c
index 2e21459f504..3fbd84fcb80 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2117,12 +2117,16 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED,
/* If INSN contains an autoincrement or autodecrement, make sure that
register is not used between there and I3, and not already used in
- I3 either. Neither must it be used in PRED or SUCC, if they exist. */
+ I3 either. Neither must it be used in PRED or SUCC, if they exist.
+ Also insist that I3 not be a jump if using LRA; if it were one
+ and the incremented register were spilled, we would lose.
+ Reload handles this correctly. */
if (AUTO_INC_DEC)
for (link = REG_NOTES (insn); link; link = XEXP (link, 1))
if (REG_NOTE_KIND (link) == REG_INC
- && (reg_used_between_p (XEXP (link, 0), insn, i3)
+ && ((JUMP_P (i3) && targetm.lra_p ())
+ || reg_used_between_p (XEXP (link, 0), insn, i3)
|| (pred != NULL_RTX
&& reg_overlap_mentioned_p (XEXP (link, 0), PATTERN (pred)))
|| (pred2 != NULL_RTX
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Autoinc vs reload and LRA
2019-11-25 20:27 Autoinc vs reload and LRA Bernd Schmidt
@ 2019-11-27 1:20 ` Jeff Law
2019-11-28 1:38 ` Segher Boessenkool
1 sibling, 0 replies; 3+ messages in thread
From: Jeff Law @ 2019-11-27 1:20 UTC (permalink / raw)
To: Bernd Schmidt, GCC Patches; +Cc: Segher Boessenkool
On 11/25/19 1:22 PM, Bernd Schmidt wrote:
> So I was curious what would happen if I turned on LRA for m68k. It turns
> out my autoinc patches from the cc0 patch set expose a bug in how LRA
> handles autoincrement. While it copies the logic from reload's
> inc_for_reload, it appears to be missing the find_reloads_address code
> to ensure an autoinc address is reloaded entirely if it is part of a
> jump. LRA can reload just the register inside a POST_INC, which leads to
> creating an output reload.
>
> Hence, a new version of the autoinc changes, below. Since reload is
> known to work, we allow autoinc in jumps unless targetm.lra_p. One part
> of the patch is a fix for the earlier combine patch which was already
> checked in, the other part is a new version of the auto-inc-dec patch.
> Bootstrapped and tested on the gcc135 machine
> (powerpc64le-unknown-linux-gnu). OK?
This is OK.
jeff
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Autoinc vs reload and LRA
2019-11-25 20:27 Autoinc vs reload and LRA Bernd Schmidt
2019-11-27 1:20 ` Jeff Law
@ 2019-11-28 1:38 ` Segher Boessenkool
1 sibling, 0 replies; 3+ messages in thread
From: Segher Boessenkool @ 2019-11-28 1:38 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: GCC Patches
On Mon, Nov 25, 2019 at 09:22:55PM +0100, Bernd Schmidt wrote:
> So I was curious what would happen if I turned on LRA for m68k. It turns
> out my autoinc patches from the cc0 patch set expose a bug in how LRA
> handles autoincrement. While it copies the logic from reload's
> inc_for_reload, it appears to be missing the find_reloads_address code
> to ensure an autoinc address is reloaded entirely if it is part of a
> jump. LRA can reload just the register inside a POST_INC, which leads to
> creating an output reload.
>
> Hence, a new version of the autoinc changes, below. Since reload is
> known to work, we allow autoinc in jumps unless targetm.lra_p. One part
> of the patch is a fix for the earlier combine patch which was already
> checked in, the other part is a new version of the auto-inc-dec patch.
> Bootstrapped and tested on the gcc135 machine
> (powerpc64le-unknown-linux-gnu). OK?
Thanks :-)
Could you open a PR for the LRA problem please?
Segher
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-28 0:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-25 20:27 Autoinc vs reload and LRA Bernd Schmidt
2019-11-27 1:20 ` Jeff Law
2019-11-28 1:38 ` Segher Boessenkool
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).