Hello,
and ping please.
Thanks,
Martin
On Mon, Feb 10 2025, Martin Jambor wrote:
> Hello,
>
> among other things, IPA-SRA checks whether splitting out a bit of an
> aggregate or something passed by reference would lead into a clash
> with an already known IPA-CP constant a way which would cause problems
> later on. Unfortunately the test is done only in
> adjust_parameter_descriptions and is missing when accesses are
> propagated from callees to callers, which leads to miscompilation
> reported as PR 118243 (where the callee is a function created by
> ipa-split).
>
> The matter is then further complicated by the fact that we consider
> complex numbers as scalars even though they can be modified piecemeal
> (IPA-CP can detect and propagate the pieces separately too) which then
> confuses the parameter manipulation machinery furter.
>
> This patch simply adds the missing check to avoid the IPA-SRA
> transform in these cases too, which should be suitable for backporting
> to all affected release branches. It is a bit of a shame as in the PR
> testcase we do propagate both components of the complex number in
> question and the transformation phase could recover. I have some
> prototype patches in this direction but that is something for (a)
> stage 1.
>
> Bootstrapped and tested on x86_64-linux. OK for master and (assuming it
> applies cleanly and passes the checks there too) to all active release
> branches?
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2025-02-10 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/118243
> * ipa-sra.cc (pull_accesses_from_callee): New parameters
> caller_ipcp_ts and param_idx. Check that scalar pulled accesses would
> not clash with a known IPA-CP aggregate constant.
> (param_splitting_across_edge): Pass IPA-CP transformation summary and
> caller parameter index to pull_accesses_from_callee.
>
> gcc/testsuite/ChangeLog:
>
> 2025-02-10 Martin Jambor <mjam...@suse.cz>
>
> PR ipa/118243
> * g++.dg/ipa/pr118243.C: New test.
> ---
> gcc/ipa-sra.cc | 38 +++++++++++++++++++--------
> gcc/testsuite/g++.dg/ipa/pr118243.C | 40 +++++++++++++++++++++++++++++
> 2 files changed, 68 insertions(+), 10 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/ipa/pr118243.C
>
> diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc
> index ad80d22f8ce..5d1703ed394 100644
> --- a/gcc/ipa-sra.cc
> +++ b/gcc/ipa-sra.cc
> @@ -3640,15 +3640,19 @@ enum acc_prop_kind {ACC_PROP_DONT, ACC_PROP_COPY,
> ACC_PROP_CERTAIN};
>
> /* Attempt to propagate all definite accesses from ARG_DESC to PARAM_DESC,
> (which belongs to CALLER) if they would not violate some constraint there.
> - If successful, return NULL, otherwise return the string reason for failure
> - (which can be written to the dump file). DELTA_OFFSET is the known offset
> - of the actual argument withing the formal parameter (so of ARG_DESCS
> within
> - PARAM_DESCS), ARG_SIZE is the size of the actual argument or zero, if not
> - known. In case of success, set *CHANGE_P to true if propagation actually
> - changed anything. */
> + CALLER_IPCP_TS describes the caller, PARAM_IDX is the index of the
> parameter
> + described by PARAM_DESC. If successful, return NULL, otherwise return the
> + string reason for failure (which can be written to the dump file).
> + DELTA_OFFSET is the known offset of the actual argument withing the formal
> + parameter (so of ARG_DESCS within PARAM_DESCS), ARG_SIZE is the size of
> the
> + actual argument or zero, if not known. In case of success, set *CHANGE_P
> to
> + true if propagation actually changed anything. */
>
> static const char *
> -pull_accesses_from_callee (cgraph_node *caller, isra_param_desc *param_desc,
> +pull_accesses_from_callee (cgraph_node *caller,
> + ipcp_transformation *caller_ipcp_ts,
> + int param_idx,
> + isra_param_desc *param_desc,
> isra_param_desc *arg_desc,
> unsigned delta_offset, unsigned arg_size,
> bool *change_p)
> @@ -3673,6 +3677,17 @@ pull_accesses_from_callee (cgraph_node *caller,
> isra_param_desc *param_desc,
> continue;
>
> unsigned offset = argacc->unit_offset + delta_offset;
> +
> + if (caller_ipcp_ts && !AGGREGATE_TYPE_P (argacc->type))
> + {
> + ipa_argagg_value_list avl (caller_ipcp_ts);
> + tree value = avl.get_value (param_idx, offset);
> + if (value && ((tree_to_uhwi (TYPE_SIZE (TREE_TYPE (value)))
> + / BITS_PER_UNIT)
> + != argacc->unit_size))
> + return " propagated access would conflict with an IPA-CP constant";
> + }
> +
> /* Given that accesses are initially stored according to increasing
> offset and decreasing size in case of equal offsets, the following
> searches could be written more efficiently if we kept the ordering
> @@ -3781,6 +3796,8 @@ param_splitting_across_edge (cgraph_edge *cs)
> cgraph_node *callee = cs->callee->function_symbol (&availability);
> isra_func_summary *from_ifs = func_sums->get (cs->caller);
> gcc_checking_assert (from_ifs && from_ifs->m_parameters);
> + ipcp_transformation *caller_ipcp_ts
> + = ipcp_get_transformation_summary (cs->caller);
>
> isra_call_summary *csum = call_sums->get (cs);
> gcc_checking_assert (csum);
> @@ -3851,8 +3868,8 @@ param_splitting_across_edge (cgraph_edge *cs)
> else
> {
> const char *pull_failure
> - = pull_accesses_from_callee (cs->caller, param_desc, arg_desc,
> - 0, 0, &res);
> + = pull_accesses_from_callee (cs->caller, caller_ipcp_ts, idx,
> + param_desc, arg_desc, 0, 0, &res);
> if (pull_failure)
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
> @@ -3913,7 +3930,8 @@ param_splitting_across_edge (cgraph_edge *cs)
> else
> {
> const char *pull_failure
> - = pull_accesses_from_callee (cs->caller, param_desc, arg_desc,
> + = pull_accesses_from_callee (cs->caller, caller_ipcp_ts, idx,
> + param_desc, arg_desc,
> ipf->unit_offset,
> ipf->unit_size, &res);
> if (pull_failure)
> diff --git a/gcc/testsuite/g++.dg/ipa/pr118243.C
> b/gcc/testsuite/g++.dg/ipa/pr118243.C
> new file mode 100644
> index 00000000000..5efaa276da1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr118243.C
> @@ -0,0 +1,40 @@
> +/* { dg-do run } */
> +/* { dg-options "-O3 -std=gnu++11" } */
> +
> +using complex_t = int __complex__;
> +
> +struct A {
> + complex_t value;
> + A(double r) : value{0, r} {}
> +};
> +
> +[[gnu::noipa]]
> +void f(int a)
> +{
> + if (a != 1)
> + __builtin_abort();
> +}
> +[[gnu::noipa]] void g(const char *, int x){}
> +
> +
> +void test(const complex_t &c, const int &x) {
> + if (x < 0)
> + g("%d\n", x);
> + else
> + {
> + f( __imag__ c);
> + }
> +}
> +
> +void f1() {
> + {
> + A a{1};
> + test(a.value, 123);
> + }
> +}
> +
> +int main()
> +{
> + f1();
> + return 0;
> +}
> --
> 2.47.1
