Hi,
On Wed, Jan 12 2022, Eric Botcazou wrote:
>> Thanks for the fixes, the forgotten duplication and streaming were quite
>> embarrassing, but one reason is that there are few reverse SSO testcases
>> in the suite. Would it be difficult to add some covering the issues you
>> fixed?
>
> No, I think I should be able to cover 3 out of the 4 changes, see below.
>
>> ...is this strictly necessary? I know it is inconsistent but the
>> certain flag is fairly special. More importantly...
>
> No strong opinion, but that's really inconsistent...
>
>> > @@ -3349,6 +3346,7 @@ pull_accesses_from_callee (cgraph_node *caller,
>> > isra_param_desc *param_desc,>
>> > copy->type = argacc->type;
>> > copy->alias_ptr_type = argacc->alias_ptr_type;
>> > copy->certain = true;
>> >
>> > + copy->reverse = argacc->reverse;
>> >
>> > vec_safe_push (param_desc->accesses, copy);
>> >
>> > }
>> >
>> > else if (prop_kinds[j] == ACC_PROP_CERTAIN)
>>
>> ...earlier in the function, there is a check doing:
>>
>> if (argacc->alias_ptr_type != pacc->alias_ptr_type
>>
>> || !types_compatible_p (argacc->type, pacc->type))
>>
>> return "propagated access types would not match existing
> ones";
>>
>> Will the types_compatible_p catch the case when one type is a
>> reverse-SSO and the other is not? If not, we probably need to check
>> that the flags are the same too.
>
> That's the change I don't know how to cover and I agree that the check looks
> in order,
It might be indeed difficult to trigger the inconsistency without some
rather unsavory type casting. Basically it would mean we would have
something like...
f (struct with_reverse s)
{
use (s.field);
}
g (struct without_reverse_buth_otherwise_identical s)
{
f ((struct with_reverse) s);
}
However with LTO, in incorrect programs, this might happen even without
the typecast and so IPA might encounter the situation even without an
explicit typecast.
> but I presume that the flag still needs to be copied onto "copy"?
>
Yes, it must still be copied.
Thanks,
Martin
