Peter Bergner <berg...@linux.ibm.com> writes:
> On 4/29/20 10:13 AM, Richard Sandiford wrote:
>> If this is a bug we need to fix for GCC 10 then how about this:
>
> I do think this is important to fix. Mike Meissner just switched our
> -mcpu=future code to enable -mpcrel by default in GCC10, so we're going
> to see a lot more of these types of addresses than before.
OK.
>> (1) change the MEM case to something like:
>>
>> {
>> bool addr_changed = false;
>> rtx addr = cse_process_notes (XEXP (x, 0), x, &addr_changed);
>> if (addr_changed && validate_change (x, &XEXP (x, 0), addr, 0))
>> *changed = true;
>> return x;
>> }
>>
>> which makes it safe to do the validation only at the top level
>> of the MEM.
>>
>> (2) make the:
>>
>> for (i = 0; i < GET_RTX_LENGTH (code); i++)
>> if (fmt[i] == 'e')
>>
>> loop assign directly to XEXP (x, i) if the new rtx is different,
>> instead of using validate_change. This ensures that temporarily
>> invalid rtl doesn't get verified.
>>
>> (passing a null object to validate_change would have the same effect,
>> but that feels a bit hacky)
>>
>> (3) use the simplify_binary_operator thing above, since that at least
>> should be safe, even if it potentially leaves similar bugs unfixed
>> for other operators.
>>
>> Sorry for the runaround :-(
>
> No problem. I appreciate the feedback and I agree we want to get this right.
> I'll implement the above and report back. Thanks!
Sigh. And now of course I realise that that too is flawed. If we make
a replacement on a subexpression and the containing expression isn't
simplified, the validate_change for the MEM will end up being a no-op,
even if it shouldn't be. So in one case we really need the recursive
validate_changes, and in one case we really want to avoid them.
I think at this point it would be better to go for the
simplify_replace_fn_rtx thing I mentioned in the original review,
rather than try to hack at the current code even more. What do you
think about the attached? Testing in progress.
(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. :-()
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
