On Thu, 11 Jan 2018, Jakub Jelinek wrote:
> Hi!
>
> My recent C FE lval folding improvements apparently broke OpenMP/OpenACC
> handling of constant size VLAs, e.g.
> const int size = 4096;
> int array[size];
> The problem is that the OpenMP/ACC gimplification/lowering/expansion relies
> on TREE_CODE (DECL_SIZE_UNIT ()) != INTEGER_CST, or !TREE_CONSTANT
> (TYPE_SIZE_UNIT ()) or variably_modified_type_p predicates to find VLAs that
> have been processed by gimplify_vla_decl and need special handling.
> The problem is that as the C FE got better at folding stuff, we end up with
> decls that have non-INTEGER_CST DECL_SIZE_UNIT which after
> gimplify_one_sizepos becomes INTEGER_CST, so the only way to distinguish it
> from non-vlas is that it has certain DECL_VALUE_EXPR. There are many other
> reasons why something could have a DECL_VALUE_EXPR, so checking that sounds
> very error-prone.
>
> This patch instead treats such arrays as non-VLAs, by first gimplifying
> DECL_SIZE_UNIT and only then checking if it is non-INTEGER_CST.
> It will surely result in better code, even when not using OpenMP/OpenACC,
> some people think const in C is the same thing as C++, on the other side,
> there might be some carefully crafted code that uses these weirdo constant
> VLAs and expect them to be deallocated every time.
> { const int n = 4096*4096; int vla[n]; foo (vla); }
> { const int n = 4096*4096; float vla[n]; bar (vla); }
> ...
> even when for some reason we wouldn't be able to share the constant size
> array stack memory.
>
> Is this acceptable optimization anyway? Bootstrapped/regtested on
> x86_64-linux and i686-linux.
>
> The alternative would be to add some bit on the decls that would tell the
> rest of gimplifier and middle-end that gimplify_vla_decl has been done on it
> and it needs to be treated specially.
Or another workaround would be to make sure non-constant sizepos
stay non-constant by doing sth like the following? FEs can mark
expressions as TREE_CONSTANT if they don't want this behavior
for things that might optimize to constants during gimplification.
Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c (revision 256535)
+++ gcc/gimplify.c (working copy)
@@ -12567,9 +12567,13 @@ gimplify_one_sizepos (tree *expr_p, gimp
*expr_p = unshare_expr (expr);
+ bool non_constant_p = ! TREE_CONSTANT (*expr_p);
/* SSA names in decl/type fields are a bad idea - they'll get reclaimed
if the def vanishes. */
gimplify_expr (expr_p, stmt_p, NULL, is_gimple_val, fb_rvalue, false);
+ if (non_constant_p
+ && is_gimple_constant (*expr_p))
+ *expr_p = get_initialized_tmp_var (*expr_p, stmt_p, NULL, false);
}
/* Gimplify the body of statements of FNDECL and return a GIMPLE_BIND
node
> 2018-01-11 Jakub Jelinek <ja...@redhat.com>
>
> PR libgomp/83590
> * gimplify.c (gimplify_vla_decl): Don't call gimplify_one_sizepos
> for DEC_SIZE and DECL_SIZE_UNIT here...
> (gimplify_decl_expr): ... but here before testing if DECL_SIZE_UNIT
> is INTEGER_CST.
> (gimplify_target_expr): And here too. Call gimplify_type_sizes
> always, not just for VLAs. Check DECL_SIZE_UNIT rather than DECL_SIZE
> for consistency with gimplify_decl_expr.
>
> * gcc.target/i386/stack-check-18.c (main): Drop const from size
> variable.
>
> --- gcc/gimplify.c.jj 2018-01-03 10:19:55.887534074 +0100
> +++ gcc/gimplify.c 2018-01-11 10:10:37.733519519 +0100
> @@ -1587,9 +1587,6 @@ gimplify_vla_decl (tree decl, gimple_seq
> for deferred expansion. */
> tree t, addr, ptr_type;
>
> - gimplify_one_sizepos (&DECL_SIZE (decl), seq_p);
> - gimplify_one_sizepos (&DECL_SIZE_UNIT (decl), seq_p);
> -
> /* Don't mess with a DECL_VALUE_EXPR set by the front-end. */
> if (DECL_HAS_VALUE_EXPR_P (decl))
> return;
> @@ -1673,6 +1670,9 @@ gimplify_decl_expr (tree *stmt_p, gimple
> tree init = DECL_INITIAL (decl);
> bool is_vla = false;
>
> + gimplify_one_sizepos (&DECL_SIZE (decl), seq_p);
> + gimplify_one_sizepos (&DECL_SIZE_UNIT (decl), seq_p);
> +
> if (TREE_CODE (DECL_SIZE_UNIT (decl)) != INTEGER_CST
> || (!TREE_STATIC (decl)
> && flag_stack_check == GENERIC_STACK_CHECK
> @@ -6548,14 +6548,15 @@ gimplify_target_expr (tree *expr_p, gimp
> {
> tree cleanup = NULL_TREE;
>
> + if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
> + gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> + gimplify_one_sizepos (&DECL_SIZE (temp), pre_p);
> + gimplify_one_sizepos (&DECL_SIZE_UNIT (temp), pre_p);
> +
> /* TARGET_EXPR temps aren't part of the enclosing block, so add it
> to the temps list. Handle also variable length TARGET_EXPRs. */
> - if (TREE_CODE (DECL_SIZE (temp)) != INTEGER_CST)
> - {
> - if (!TYPE_SIZES_GIMPLIFIED (TREE_TYPE (temp)))
> - gimplify_type_sizes (TREE_TYPE (temp), pre_p);
> - gimplify_vla_decl (temp, pre_p);
> - }
> + if (TREE_CODE (DECL_SIZE_UNIT (temp)) != INTEGER_CST)
> + gimplify_vla_decl (temp, pre_p);
> else
> {
> /* Save location where we need to place unpoisoning. It's possible
> --- gcc/testsuite/gcc.target/i386/stack-check-18.c.jj 2018-01-03
> 21:21:38.707907706 +0100
> +++ gcc/testsuite/gcc.target/i386/stack-check-18.c 2018-01-11
> 19:10:11.933703006 +0100
> @@ -7,7 +7,7 @@ int f1 (char *);
> int
> f2 (void)
> {
> - const int size = 4096;
> + int size = 4096;
> char buffer[size];
> return f1 (buffer);
> }
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
