<[email protected]> writes:
> From: Dhruv Chawla <[email protected]>
>
> r17-4816-gb24418a2212866 gives a call whose result is discarded a placeholder
> lhs using create_tmp_var, which gives a VAR_DECL. This ends up setting
> ssa_renaming_needed and the subsequent fold passes do not set TODO_update_ssa,
> so execute_todo ICEs. This is only set for register types and not for
> aggregates, like svfloat32x2_t, which cannot be made via make_ssa_name
> regardless. Thus, only generate the SSA name when is_gimple_reg_type is
> true.
>
> This needs the fold to run after SSA construction. In the testcase the second
> operand is not yet a constant when intrinsics are first folded from
> gimple-low, so the fold only succeeds once CCP has executed.
>
> Bootstrapped and regtested on aarch64-linux-gnu.
>
> Signed-off-by: Dhruv Chawla <[email protected]>
>
> gcc/ChangeLog:
>
> PR target/127771
> * config/aarch64/aarch64-acle-builtins.cc: Include ssa.h.
> (gimple_folder::fold): Make an SSA name for the placeholder when
> is_gimple_reg_type and release it when unused.
>
> gcc/testsuite/ChangeLog:
>
> PR target/127771
> * gcc.target/aarch64/sve/acle/general/pr127771.c: New test.
Sorry to come back to this, but r17-4816-gb24418a2212866 happened in the
context of intrinsics that have no RTL expansion. I thought the premise
was that that was safe because gimple lowering would always attempt to fold
the intrinsics. (Gimple lowering is special in that AFAIK it happens for
all compilations, with no way of turning it off.)
If in fact the lowering can happen later, while in SSA form, then are we
sure that there's always a guarantee that it happens at all?
I'm not sure how safe the approach in the patch is. E.g. I got the
impression that checking SSA_NAME_DEF_STMT for null was usually a sign
that we were papering over problems elsewhere.
This just feels pretty fragile to me. I don't mind having a principle
that certain target intrinsics need to be lowered to other gimple and
not survive until expand. But I think we should formalise it more,
rather than trying to hide it in target code.
Thanks,
Richard
> ---
> gcc/config/aarch64/aarch64-acle-builtins.cc | 37 ++++++++++++++++---
> .../aarch64/sve/acle/general/pr127771.c | 21 +++++++++++
> 2 files changed, 52 insertions(+), 6 deletions(-)
> create mode 100644
> gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr127771.c
>
> diff --git a/gcc/config/aarch64/aarch64-acle-builtins.cc
> b/gcc/config/aarch64/aarch64-acle-builtins.cc
> index 5abcc01b96bf..238570d9becd 100644
> --- a/gcc/config/aarch64/aarch64-acle-builtins.cc
> +++ b/gcc/config/aarch64/aarch64-acle-builtins.cc
> @@ -46,6 +46,7 @@
> #include "gimple-fold.h"
> #include "langhooks.h"
> #include "stringpool.h"
> +#include "ssa.h"
> #include "attribs.h"
> #include "value-range.h"
> #include "tree-ssanames.h"
> @@ -3142,16 +3143,40 @@ gimple_folder::fold ()
> /* Create a placeholder variable for the result.
> This allows implementers of `fold` to assume `lhs` will never be null.
> If the result is unused, it will be optimized away later. */
> + tree placeholder = NULL_TREE;
> if (!lhs && TREE_TYPE (gimple_call_fntype (call)) != void_type_node)
> - lhs = create_tmp_var (TREE_TYPE (gimple_call_fntype (call)));
> + {
> + tree type = TREE_TYPE (gimple_call_fntype (call));
> +
> + /* PR target/127771: A register result has to be an SSA name as
> VAR_DECLs
> + created via create_tmp_var set ssa_renaming_needed and the fold passes
> + do not set TODO_update_ssa, so execute_todo ICEs. Aggregates cannot be
> + SSA names at all, but they do not set that flag either, so a
> + declaration is both necessary and safe for them. */
> + if (is_gimple_reg_type (type))
> + lhs = placeholder = make_ssa_name (type);
> + else
> + lhs = create_tmp_var (type);
> + }
>
> /* First try some simplifications that are common to many functions. */
> - if (auto *call = redirect_pred_x ())
> - return call;
> - if (auto *call = fold_pfalse ())
> - return call;
> + gimple *stmt = redirect_pred_x ();
> + if (!stmt)
> + stmt = fold_pfalse ();
> + if (!stmt)
> + stmt = base->fold (*this);
> +
> + /* Release a placeholder that nothing defines and nothing uses, because
> + gimple_build_assign does not set the definition until insertion and
> + convert_and_fold returns a different statement from the one it inserts.
> */
> + if (placeholder && (!stmt || gimple_get_lhs (stmt) != placeholder))
> + {
> + gimple *def = SSA_NAME_DEF_STMT (placeholder);
> + if (has_zero_uses (placeholder) && (!def || gimple_nop_p (def)))
> + release_ssa_name (placeholder);
> + }
>
> - return base->fold (*this);
> + return stmt;
> }
>
> /* Force EXPR to be a GIMPLE value, putting it into a new variable if
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr127771.c
> b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr127771.c
> new file mode 100644
> index 000000000000..0a024ca4954f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve/acle/general/pr127771.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1" } */
> +
> +#include <arm_sve.h>
> +
> +/* PR target/127771 */
> +
> +void
> +g (svbool_t p, svuint32_t a)
> +{
> + svmul_x (p, a, svdup_u32 (1));
> +}
> +
> +/* The placeholder for a discarded result cannot be an SSA name when the
> + return type is an aggregate. */
> +
> +void
> +h (svuint32_t a, svuint32_t b)
> +{
> + svcreate2_u32 (a, b);
> +}
