Peter Bergner <berg...@linux.ibm.com> writes:
> On 4/28/20 5:39 PM, Richard Sandiford wrote:
>> If we use simplify_gen_binary then I don't think we need to validate
>> each change individually. It should be enough to do something like:
>>
>> x0 = cse_process_notes (XEXP (x, 0), object, changed);
>> x1 = cse_process_notes (XEXP (x, 1), object, changed);
>> if (x0 == XEXP (x, 0) && x1 == XEXP (x, 1))
>> return x;
>> return simplify_gen_binary (code, GET_MODE (x), x0, x1);
>
> Ok, I wasn't sure whether simplify_gen_binary verified its changes or not.
>
>> Alternatively, in the hope of reducing redundant rtl, I guess we could
>> add the switch statement after the format walk and do:
>>
>> switch (GET_RTX_CLASS (code))
>> {
>> case RTX_BIN_ARITH:
>> case RTX_COMM_ARITH:
>> if (rtx new_rtx = simplify_binary (code, GET_MODE (x),
>> XEXP (x, 0), XEXP (x, 1)))
>> return new_rtx;
>> break;
>> default:
>> break;
>> }
To answer your later question, I meant simplify_binary_operation rather
than simplify_binary here. Sorry, I always forget that the simplify_gen_*
and non-gen versions use different naming schemes.
But then I guess we're effectively back to acting like simplify_rtx,
as in your original patch, but without checking whether we're in a MEM.
The traditional problem with doing recursive replacement followed by
simplification is that we can lose the original mode for the operands
of unary operators. E.g. (zero_extend:DI (reg:M X)) can become
(zero_extend:DI (const_int -1)), which isn't well-formed.
So calling simplify_rtx for all rtxes (not just binary ones)
could lead to problems. More below.
> Doesn't moving this after the format walk, create the situation of
> producing constant expressions without (const:P ...) and then fixing
> them up after the fact like my original patch did? That's why I placed
> the previous version before the format walk, but if you're ok with that
> now, then that's fine with me. That said, the PLUS will be wrapped in
> a (const:P ...) before it's substituted into the MEM, so maybe it was
> just the MEM you were worried about?
To be honest, when reviewing the original patch, I think I'd missed that
cse_process_notes and cse_process_notes_1 were co-recursive, and so
every level of the MEM would be simplified by your simplify_rtx call.
I originally thought it was building up a new MEM and only then
simplifying it.
But yeah, like you say, the problem is that if we just add the
simplification on top, we're still validating the original unsimplified
replacement beforehand, so there's still a potential for non-canonical
rtl to seep into the validation routine before it gets corrected by
a parent call. That might cause problems in future, even if it
doesn't yet.
Ugh.
Unfortunately, there doesn't seem to be a perfect off-the-shelf
routine to use here. validate_replace_rtx_part looks like it has
the right kind of idea, but can only handle a single "from" and "to"
value. simplify_replace_fn_rtx can handle general replacements,
but generates more garbage rtl and doesn't verify the new MEMs.
If this is a bug we need to fix for GCC 10 then how about this:
(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 :-(
Hopefully this area is something we can clean up for GCC 11.
Thanks,
Richard
