Richard Sandiford <richard.sandif...@arm.com> writes:
> Peter Bergner <berg...@linux.ibm.com> writes:
>> On 4/29/20 4:15 PM, Peter Bergner wrote:
>>> On 4/29/20 3:28 PM, Richard Sandiford wrote:
>>>> (Sorry for going ahead and writing an alternative patch, since if we do
>>>> go for this, I guess the earlier misdirections will have wasted two days
>>>> of your time. But it seemed like I was just never going to think about
>>>> this PR properly unless I actually tried to write something. :-()
>>>
>>> No worries from me! I'm just glad to see this fixed before the release.
>>> I'll kill off a bootstrap and regtest on powerpc64le-linux too, in addition
>>> to your tests (arm & x86_64?). Thanks for your help with this!
>>
>> My bootstrap and regtesting of your patch on powerpc64le-linux was clean.
>
> Thanks. aarch64-linux-gnu and x86_64-linux-gnu bootstrap & regtests
> also came back clean. I'll kick off an arm-linux-gnueabihf one too
> just to be safe.
FTR, that came back clean too.
> I guess at this point it needs a review from someone else though.
> Jeff, WDYT? Attached again below, this time without the shonky mime type.
Here's a different attempt to attach the patch (sorry -- experimenting
with different ways of attaching the patch inline, to see how the ML
archive reacts.)
Richard
>From 39e20b51af8f977a1786ef5c15af80e47415d352 Mon Sep 17 00:00:00 2001
From: Richard Sandiford <richard.sandif...@arm.com>
Date: Wed, 29 Apr 2020 20:38:13 +0100
Subject: [PATCH] cse: Use simplify_replace_fn_rtx to process notes [PR94740]
cse_process_notes did a very simple substitution, which in the wrong
circumstances could create non-canonical RTL and invalid MEMs.
Various sticking plasters have been applied to cse_process_notes_1
to handle cases like ZERO_EXTEND, SIGN_EXTEND and UNSIGNED_FLOAT,
but I think this PR is a plaster too far.
The code is trying hard to avoid creating unnecessary rtl, which of
course is a good thing. If we continue to do that, then we can end
up changing subexpressions while keeping the containing rtx.
This in turn means that validate_change will be a no-op on the
containing rtx, even if its contents have changed. So in these
cases we have to apply validate_change to the individual subexpressions.
On the other hand, if we always apply validate_change to the
individual subexpressions, we'll end up calling validate_change
on something before it has been simplified and canonicalised.
And that's one of the situations we're trying to avoid.
There might be a middle ground in which we queue the validate_changes
as part of a group, and so can cancel the pending validate_changes
for subexpressions if there's a change in the outer expression.
But that seems even more ad-hoc than the current code.
It would also be quite an invasive change.
I think the best thing is just to hook into the existing
simplify_replace_fn_rtx function, keeping the REG and MEM handling
from cse_process_notes_1 essentially unchanged. It can generate
more redundant rtl when a simplification takes place, but it has
the advantage of being relative well-used code (both directly
and via simplify_replace_rtx).
2020-04-29 Richard Sandiford <richard.sandif...@arm.com>
gcc/
PR rtl-optimization/94740
* cse.c (cse_process_notes_1): Replace with...
(cse_process_note_1): ...this new function, acting as a
simplify_replace_fn_rtx callback to process_note. Handle only
REGs and MEMs directly. Validate the MEM if cse_process_note
changes its address.
(cse_process_notes): Replace with...
(cse_process_note): ...this new function.
(cse_extended_basic_block): Update accordingly, iterating over
the register notes and passing individual notes to cse_process_note.
---
gcc/cse.c | 118 ++++++++++++++++--------------------------------------
1 file changed, 34 insertions(+), 84 deletions(-)
diff --git a/gcc/cse.c b/gcc/cse.c
index 5aaba8d80e0..36bcfc354d8 100644
--- a/gcc/cse.c
+++ b/gcc/cse.c
@@ -585,7 +585,6 @@ static void cse_insn (rtx_insn *);
static void cse_prescan_path (struct cse_basic_block_data *);
static void invalidate_from_clobbers (rtx_insn *);
static void invalidate_from_sets_and_clobbers (rtx_insn *);
-static rtx cse_process_notes (rtx, rtx, bool *);
static void cse_extended_basic_block (struct cse_basic_block_data *);
extern void dump_class (struct table_elt*);
static void get_cse_reg_info_1 (unsigned int regno);
@@ -6222,75 +6221,28 @@ invalidate_from_sets_and_clobbers (rtx_insn *insn)
}
}
-/* Process X, part of the REG_NOTES of an insn. Look at any REG_EQUAL notes
- and replace any registers in them with either an equivalent constant
- or the canonical form of the register. If we are inside an address,
- only do this if the address remains valid.
+static rtx cse_process_note (rtx);
- OBJECT is 0 except when within a MEM in which case it is the MEM.
+/* A simplify_replace_fn_rtx callback for cse_process_note. Process X,
+ part of the REG_NOTES of an insn. Replace any registers with either
+ an equivalent constant or the canonical form of the register.
+ Only replace addresses if the containing MEM remains valid.
- Return the replacement for X. */
+ Return the replacement for X, or null if it should be simplified
+ recursively. */
static rtx
-cse_process_notes_1 (rtx x, rtx object, bool *changed)
+cse_process_note_1 (rtx x, const_rtx, void *)
{
- enum rtx_code code = GET_CODE (x);
- const char *fmt = GET_RTX_FORMAT (code);
- int i;
-
- switch (code)
+ if (MEM_P (x))
{
- case CONST:
- case SYMBOL_REF:
- case LABEL_REF:
- CASE_CONST_ANY:
- case PC:
- case CC0:
- case LO_SUM:
+ validate_change (x, &XEXP (x, 0), cse_process_note (XEXP (x, 0)), false);
return x;
+ }
- case MEM:
- validate_change (x, &XEXP (x, 0),
- cse_process_notes (XEXP (x, 0), x, changed), 0);
- return x;
-
- case EXPR_LIST:
- if (REG_NOTE_KIND (x) == REG_EQUAL)
- XEXP (x, 0) = cse_process_notes (XEXP (x, 0), NULL_RTX, changed);
- /* Fall through. */
-
- case INSN_LIST:
- case INT_LIST:
- if (XEXP (x, 1))
- XEXP (x, 1) = cse_process_notes (XEXP (x, 1), NULL_RTX, changed);
- return x;
-
- case SIGN_EXTEND:
- case ZERO_EXTEND:
- case SUBREG:
- {
- rtx new_rtx = cse_process_notes (XEXP (x, 0), object, changed);
- /* We don't substitute VOIDmode constants into these rtx,
- since they would impede folding. */
- if (GET_MODE (new_rtx) != VOIDmode)
- validate_change (object, &XEXP (x, 0), new_rtx, 0);
- return x;
- }
-
- case UNSIGNED_FLOAT:
- {
- rtx new_rtx = cse_process_notes (XEXP (x, 0), object, changed);
- /* We don't substitute negative VOIDmode constants into these rtx,
- since they would impede folding. */
- if (GET_MODE (new_rtx) != VOIDmode
- || (CONST_INT_P (new_rtx) && INTVAL (new_rtx) >= 0)
- || (CONST_DOUBLE_P (new_rtx) && CONST_DOUBLE_HIGH (new_rtx) >= 0))
- validate_change (object, &XEXP (x, 0), new_rtx, 0);
- return x;
- }
-
- case REG:
- i = REG_QTY (REGNO (x));
+ if (REG_P (x))
+ {
+ int i = REG_QTY (REGNO (x));
/* Return a constant or a constant register. */
if (REGNO_QTY_VALID_P (REGNO (x)))
@@ -6309,26 +6261,19 @@ cse_process_notes_1 (rtx x, rtx object, bool *changed)
/* Otherwise, canonicalize this register. */
return canon_reg (x, NULL);
-
- default:
- break;
}
- for (i = 0; i < GET_RTX_LENGTH (code); i++)
- if (fmt[i] == 'e')
- validate_change (object, &XEXP (x, i),
- cse_process_notes (XEXP (x, i), object, changed), 0);
-
- return x;
+ return NULL_RTX;
}
+/* Process X, part of the REG_NOTES of an insn. Replace any registers in it
+ with either an equivalent constant or the canonical form of the register.
+ Only replace addresses if the containing MEM remains valid. */
+
static rtx
-cse_process_notes (rtx x, rtx object, bool *changed)
+cse_process_note (rtx x)
{
- rtx new_rtx = cse_process_notes_1 (x, object, changed);
- if (new_rtx != x)
- *changed = true;
- return new_rtx;
+ return simplify_replace_fn_rtx (x, NULL_RTX, cse_process_note_1, NULL);
}
@@ -6623,14 +6568,19 @@ cse_extended_basic_block (struct cse_basic_block_data
*ebb_data)
{
/* Process notes first so we have all notes in canonical forms
when looking for duplicate operations. */
- if (REG_NOTES (insn))
- {
- bool changed = false;
- REG_NOTES (insn) = cse_process_notes (REG_NOTES (insn),
- NULL_RTX, &changed);
- if (changed)
- df_notes_rescan (insn);
- }
+ bool changed = false;
+ for (rtx note = REG_NOTES (insn); note; note = XEXP (note, 1))
+ if (REG_NOTE_KIND (note) == REG_EQUAL)
+ {
+ rtx newval = cse_process_note (XEXP (note, 0));
+ if (newval != XEXP (note, 0))
+ {
+ XEXP (note, 0) = newval;
+ changed = true;
+ }
+ }
+ if (changed)
+ df_notes_rescan (insn);
cse_insn (insn);
--
2.17.1
