Hi Eric,
On Tue, Jan 11 2022, Eric Botcazou via Gcc-patches wrote:
> Hi,
>
> we recently received the report that the IPA-SRA pass introduced in GCC 10
> does not always play nice with the reverse scalar storage order that can be
> used in structures/records/unions. Reading the code, the pass apparently
> correctly detects it but fails to propagate the information to the rewriting
> phase in some cases and, in particular, does not stream it for LTO.
>
> The attached patch is a tentative fix for these issues spotted essentially by
> code reading. It also contains various minor tweaks left and right.
>
> Bootstrapped/regtested on x86-64/Linux, OK for mainline, 11 and 10 branches?
>
>
> 2022-01-11 Eric Botcazou <ebotca...@adacore.com>
>
> * ipa-param-manipulation.c (ipa_dump_adjusted_parameters): Dump
> reverse flag as "reverse" for the sake of consistency.
> * ipa-sra.c: Fix copyright year.
> (ipa_sra_function_summaries::duplicate): Copy the reverse flag.
> (dump_isra_access): Remove confusing dump line.
> (isra_write_node_summary): Write the reverse flag.
> (isra_read_node_info): Read it.
> (pull_accesses_from_callee): Copy it.
Thanks for the fixes, the forgotten duplication and streaming were quite
embarrassing, but one reason is that there are few reverse SSO testcases
in the suite. Would it be difficult to add some covering the issues you
fixed?
Apart from that...
> @@ -664,8 +663,6 @@ dump_isra_access (FILE *f, param_access *access)
> print_generic_expr (f, access->alias_ptr_type);
> if (access->certain)
> fprintf (f, ", certain");
> - else
> - fprintf (f, ", not-certain");
> if (access->reverse)
> fprintf (f, ", reverse");
> fprintf (f, "\n");
...is this strictly necessary? I know it is inconsistent but the
certain flag is fairly special. More importantly...
> @@ -3349,6 +3346,7 @@ pull_accesses_from_callee (cgraph_node *caller,
> isra_param_desc *param_desc,
> copy->type = argacc->type;
> copy->alias_ptr_type = argacc->alias_ptr_type;
> copy->certain = true;
> + copy->reverse = argacc->reverse;
> vec_safe_push (param_desc->accesses, copy);
> }
> else if (prop_kinds[j] == ACC_PROP_CERTAIN)
...earlier in the function, there is a check doing:
if (argacc->alias_ptr_type != pacc->alias_ptr_type
|| !types_compatible_p (argacc->type, pacc->type))
return "propagated access types would not match existing ones";
Will the types_compatible_p catch the case when one type is a
reverse-SSO and the other is not? If not, we probably need to check
that the flags are the same too.
Thanks a gain for looking into this.
Martin
