On Mon, 26 Apr 2021, Richard Sandiford wrote:
> Qing Zhao <qing.z...@oracle.com> writes:
> >>> @@ -1831,6 +2000,17 @@ gimplify_decl_expr (tree *stmt_p, gimple_seq
> >>> *seq_p)
> >>> as they may contain a label address. */
> >>> walk_tree (&init, force_labels_r, NULL, NULL);
> >>> }
> >>> + /* When there is no explicit initializer, if the user requested,
> >>> + We should insert an artifical initializer for this automatic
> >>> + variable for non vla variables. */
> >>
> >> I think we should explain why we can skip VLAs here.
> >
> > VLA is handled in another place already, it should be initialized with
> > calls to memset/memcpy.
>
> Yeah, what I meant here was that the comment should explain the
> difference between the handling of VLAs and non-VLAs. It's fairly
> obvious when reading the patch, but it won't be as obvious once the
> patch is applied.
>
> >>> + children are to be processed. TOP_OFFSET is the offset of the
> >>> processed
> >>> + subtree which has to be subtracted from offsets of individual
> >>> accesses to
> >>> + get corresponding offsets for AGG. GSI is a statement iterator used
> >>> to place
> >>> + the new statements. */
> >>> +static void
> >>> +generate_subtree_deferred_init (struct access *access, tree agg,
> >>> + enum auto_init_type init_type,
> >>> + HOST_WIDE_INT top_offset,
> >>> + gimple_stmt_iterator *gsi,
> >>> + location_t loc)
> >>> +{
> >>> + do
> >>> + {
> >>> + if (access->grp_to_be_replaced)
> >>> + {
> >>> + tree repl = get_access_replacement (access);
> >>> + tree init_type_node
> >>> + = build_int_cst (integer_type_node, (int) init_type);
> >>> + gimple *call = gimple_build_call_internal (IFN_DEFERRED_INIT, 2,
> >>> + repl, init_type_node);
> >>> + gimple_call_set_lhs (call, repl);
> >>
> >> AFAICT “access” is specifically for the lhs of the original call.
> >> So there seems to be an implicit assumption here that the lhs of the
> >> original call is the same as the first argument of the original call.
> >> Is that guaranteed/required?
> >
> > For call to DEFFERED_INIT, yes, this is guaranteed.
>
> OK, in that case…
>
> >> If so, I think it's something that
> >> tree-cfg.c should check. It might also be worth having an assertion
> >> in sra_modify_deferred_init.
> > I can definitely add an assertion to make sure this.
>
> …I think we need the tree-cfg.c check too. Having the check there
> ensures that the invariant is maintained throughout gimple.
>
> >>> + gimple_set_location (call, loc);
> >>> +
> >>> + sra_stats.subtree_deferred_init++;
> >>> + }
> >>> + else if (access->grp_to_be_debug_replaced)
> >>> + {
> >>> + /* FIXME, this part might have some issue. */
> >>> + tree drhs = build_debug_ref_for_model (loc, agg,
> >>> + access->offset - top_offset,
> >>> + access);
> >>> + gdebug *ds = gimple_build_debug_bind (get_access_replacement (access),
> >>> + drhs, gsi_stmt (*gsi));
> >>> + gsi_insert_before (gsi, ds, GSI_SAME_STMT);
> >>
> >> Would be good to fix the FIXME :-)
> >
> > This is the part I am not very sure, so I added the FIXME in order to get
> > more review and suggestion
> > to make sure it. -:)
> >>
> >> I guess the thing we need to decide here is whether -ftrivial-auto-var-init
> >> should affect debug-only constructs too.
> >
> > Where can I get more details on Debug-only constructs ?
>
> What I meant by “debug-only construct” is a piece of source-level data
> (in this case a field of an aggregate) that has been optimised out of
> the executable code but still exists in debug stmts. AIUI that's what
> the code above is handling.
>
> >>> @@ -4863,6 +4863,29 @@ find_func_aliases_for_builtin_call (struct
> >>> function *fn, gcall *t)
> >>> return false;
> >>> }
> >>>
> >>> +static void
> >>> +find_func_aliases_for_deferred_init (gcall *t)
> >>> +{
> >>> + tree lhsop = gimple_call_lhs (t);
> >>> + enum auto_init_type init_type
> >>> + = (enum auto_init_type) TREE_INT_CST_LOW (gimple_call_arg (t, 1));
> >>> + auto_vec<ce_s, 2> lhsc;
> >>> + auto_vec<ce_s, 4> rhsc;
> >>> + struct constraint_expr temp;
> >>> +
> >>> + get_constraint_for (lhsop, &lhsc);
> >>> + if (init_type == AUTO_INIT_ZERO && flag_delete_null_pointer_checks)
> >>> + temp.var = nothing_id;
> >>> + else
> >>> + temp.var = nonlocal_id;
> >>> + temp.type = ADDRESSOF;
> >>> + temp.offset = 0;
> >>> + rhsc.safe_push (temp);
> >>> +
> >>> + process_all_all_constraints (lhsc, rhsc);
> >>> + return;
> >>> +}
> >>> +
> >>
> >> What's the reasoning behind doing it like this? AFAICT the result
> >> of the call doesn't validly alias anything, regardless of the init type.
> >> Even if there happens to be a valid decl at the address given by the
> >> chosen fill pattern, there's no expectation that accesses to that
> >> decl will be coherent wrt accesses via the result of the call.
> >
> > So, you mean the above change will not have any impact at all?
>
> Stepping back a bit first: why did you add the code? What case
> it is trying to handle? Perhaps I misunderstood the motivation.
>
> In the above reply I meant more that the code seems unnecessarily
> pessimistic. It looks like it's based on:
>
> /* x = integer is all glommed to a single variable, which doesn't
> point to anything by itself. That is, of course, unless it is an
> integer constant being treated as a pointer, in which case, we
> will return that this is really the addressof anything. This
> happens below, since it will fall into the default case. The only
> case we know something about an integer treated like a pointer is
> when it is the NULL pointer, and then we just say it points to
> NULL.
>
> Do not do that if -fno-delete-null-pointer-checks though, because
> in that case *NULL does not fail, so it _should_ alias *anything.
> It is not worth adding a new option or renaming the existing one,
> since this case is relatively obscure. */
> if ((TREE_CODE (t) == INTEGER_CST
> && integer_zerop (t))
> /* The only valid CONSTRUCTORs in gimple with pointer typed
> elements are zero-initializer. But in IPA mode we also
> process global initializers, so verify at least. */
> || (TREE_CODE (t) == CONSTRUCTOR
> && CONSTRUCTOR_NELTS (t) == 0))
> {
> if (flag_delete_null_pointer_checks)
> temp.var = nothing_id;
> else
> temp.var = nonlocal_id;
> temp.type = ADDRESSOF;
> temp.offset = 0;
> results->safe_push (temp);
> return;
> }
>
> But if the user writes:
>
> intptr_t x = 0;
> *(int *)x = 42;
>
> and compiles with -fno-delete-null-pointer-checks, then we have to
> assume that the code is valid and is accessing a global decl that the
> user knows is at address 0. So in that case we need the &nonlocal_id
> constraint.
>
> But even with the new option, I don't think:
>
> intptr_t x;
> *(int *)x = 42;
>
> is well-defined in the same way. Maybe there's an argument that using
> -fno-delete-null-pointer-checks is such a niche case that we might as
> well treat it as though it were well-defined. That'd feel to me like
> a half-measure though. It comes back to Richard Smith's argument
> that we shouldn't try to create a new dialect of C/C++. We're
> explicitly not trying to make:
>
> intptr_t x;
>
> and
>
> intptr_t x = 0;
>
> equivalent in all respects. And if we were trying to make them
> equivalent, we'd need to do much more than this.
>
> The same applies to the pattern case. If “x” is initialised to a pattern
> that happens to point to a real decl, we don't have to preserve the
> order of accesses to the decl wrt accesses to “*x” (especially since
> we're hoping that “*x” will trap).
>
> I think for aliasing purposes, the .DEFERRED_INIT return value is still
> analogous to an undefined SSA name, even though we will later generate
> code to initialise it.
(only replying to this part, I'll look at the next revised patch series)
Since .DEFERRED_INIT does not produce any pointers and is not a real
initialization you don't need to do anything in PTA - but you might
want to ignore it to not pessimize it by the default handling. Thus
+ if (gimple_call_internal_p (t, IFN_DEFERRED_INIT))
+ {
+ find_func_aliases_for_deferred_init (t);
+ return;
+ }
should simply return and find_func_aliases_for_deferred_init can be
removed.
> >> In fact it might be simpler to have something like:
> >>
> >> if (POINTER_TYPE_P (type) && TYPE_PRECISION (type) < 64)
> >> return build_int_cstu (type, 0xAA);
> >>
> >> if (INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type))
> >> ...the wi::from_buffer thing above...;
> >
> > Okay.
> >>
> >> I'm not sure if this makes sense for NULLPTR_TYPE.
> >
> > Is there a good testing case in gcc test suite about NULLPTR_TYPE that I
> > can take a look?
>
> I'm guessing you'll need to construct a new one to exercise this code path.
>
> >>> + case RECORD_TYPE:
> >>> + {
> >>> + tree field;
> >>> + tree field_value;
> >>> + vec<constructor_elt, va_gc> *v = NULL;
> >>> + for (field= TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> >>> + {
> >>> + if (TREE_CODE (field) != FIELD_DECL)
> >>> + continue;
> >>> + /* if the field is a variable length array, it should be the last
> >>> + field of the record, and no need to initialize. */
> >>
> >> Why doesn't it need to be initialized though?
> >
> > My understanding is, the compiler will not allocate memory for the latest
> > field of the record if its a VLA, it’s the user’s responsibility to
> > allocate
> > Memory for it. Therefore, compiler doesn’t need to initialize it.
>
> Ah, OK. This sounds like the behaviour for flexible array members rather
> than VLA members. E.g. GCC allows:
>
> void bar (int *);
> int
> foo (int a)
> {
> // VLA in struct
> struct { int x[a]; } foo;
> bar (foo.x);
> }
>
> In this case the foo.x really does have “a” elements that would
> need to be initialised.
>
> I think the case you're talking about is:
>
> void bar (int *);
> int
> foo (int a)
> {
> // struct ending in a flexible array
> struct { int prefix; int x[]; } foo;
> bar (foo.x);
> }
>
> where foo.x has zero elements.
>
> >>> @@ -11950,6 +12088,72 @@ lower_bound_in_type (tree outer, tree inner)
> >>> }
> >>> }
> >>>
> >>> +/* Returns true when the given TYPE has padding inside it.
> >>> + return false otherwise. */
> >>> +bool
> >>> +type_has_padding (tree type)
> >>
> >> Would it be possible to reuse __builtin_clear_padding here?
> >
> > Not sure, where can I get more details on __builtin_clear_padding? I can
> > study a little bit more on this to make sure this.
>
> It's documented in doc/extend.texi.
>
> Thanks,
> Richard
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
