Re: [patch] Fix wrong code with small structure return on PowerPC

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Oct 3, 2017 at 8:39 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Reading the patch I think that it gets conservativeness wrong -- shouldn't
>> it be
>>
>>   if (is_definitely_initialized)
>>    {
>>       SUBREG_PROMOTED_VAR_P (temp) = 1;
>>       SUBREG_PROMOTED_SET (temp, unsignedp);
>>    }
>>
>> ?  Of course it's not easy to compute is_definitely_initialized
>> conservatively in an ad-hoc way at this place.  It should be relatively
>> straight-forward to do a conservative computation somewhere in cfgexpand.c
>> by propagating across SSA edges and recording a flag on SSA names though.
>> I assume we can take load destinations as fully initialized (should extend
>> properly) as well as call results (the ABI should extend, eventually we can
>> query the target if it does), likewise for function arguments.
>
> Yes, that's why the comment read "Try to detect if " and not "Detect if ".
>
>> On your patch:
>>
>> +             /* Try to detect if the register contains uninitialized bits.
>> */ +             if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
>> +               maybe_uninitialized = true;
>>
>> if you use ssa_undefined_value_p (ssa_name[, true]) you'd get function
>> paramters not undefined (which is probably desired?).  Likewise
>> partial initialized complex would get uninitialized (if passed , true).
>
> Ah, yes, I overlooked that.
>
>> Same inside the loop over PHI args though I wonder how pessimizing
>> it would be to simply do
>>
>>   if (ssa_undefined_value_p (ssa_name, true)
>>
>>       || gimple_code (SSA_NAME_DEF_STMT (ssa_name)) == GIMPLE_PHI)
>>
>>     maybe_uninitialized = true;
>>
>> thus make all PHIs possibly uninitialized (your code wouldn't catch a
>> chained PHI with undef arg).
>
> Too big a hammer for such a very rare bug I think.
>
>> As said, a better solution would be to do a definitely-initialized
>> mini-propagation at RTL expansion time.
>
> I'm not sure if we really need to propagate.  What about the attached patch?
> It computes at expansion time whether the partition the SSA name is a member
> of contains an undefined value and, if so, doesn't set the promoted bit for
> the SUBREG.  My gut feeling is that it's sufficient in practice.

I'll take your word for it ;)

The patch is ok.

Thanks,
Richard.

>
>         * tree-outof-ssa.h (ssaexpand): Add partitions_for_undefined_values.
>         (always_initialized_rtx_for_ssa_name_p): New predicate.
>         * tree-outof-ssa.c (remove_ssa_form): Initialize new field of SA.
>         (finish_out_of_ssa): Free new field of SA.
>         * tree-ssa-coalesce.h (get_undefined_value_partitions): Declare.
>         * tree-ssa-coalesce.c: Include tree-ssa.h.
>         (get_parm_default_def_partitions): Remove extern keyword.
>         (get_undefined_value_partitions): New function.
>         * expr.c (expand_expr_real_1) <expand_decl_rtl>: For a SSA_NAME, do
>         not set SUBREG_PROMOTED_VAR_P on the sub-register if it may contain
>         uninitialized bits.
>
> --
> Eric Botcazou