> On Apr 26, 2021, at 12:47 PM, Richard Sandiford <richard.sandif...@arm.com>
> 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.
Okay, I see, will update the comments.
>
>>>> + 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.
Okay, will add check in tree-cfg.c too.
>
>>>> + 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.
Okay, I see now.
Then, we should handle this case too, I think.
> 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.
Okay, I see now.
Will delete this part from the code.
>
>>> 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.
Will do this.
>
>>>> + 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.
Oh, Yes. My bad to use a wrong concept to confuse you.
> 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.
Yes.
Then, that part of the code currently is okay?
>
>>>> @@ -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.
Okay, will check on that.
Thanks a lot.
Qing
>
> Thanks,
> Richard
