On Tue, Oct 3, 2017 at 8:39 PM, Eric Botcazou <ebotca...@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
