on 2022/12/7 20:55, Richard Sandiford wrote:
> "Kewen.Lin" <li...@linux.ibm.com> writes:
>> Hi Richard,
>>
>> on 2022/12/7 17:16, Richard Sandiford wrote:
>>> "Kewen.Lin" <li...@linux.ibm.com> writes:
>>>> Hi,
>>>>
>>>> In the recent discussion on how to make some built-in type only valid for
>>>> some target features efficiently[1], Andrew mentioned this patch which he
>>>> made previously (Thanks!). I confirmed it can help rs6000 related issue,
>>>> and noticed PR99657 is still opened, so I think we still want this to
>>>> be reviewed.
>>>
>>> But does it work for things like:
>>>
>>> void f(foo_t *x, foo_t *y) { *x = *y; }
>>>
>>> where no variables are being created with foo_t type?
>>>
>>
>> I think it can work for this case as it touches build_indirect_ref.
>
> Ah, ok. But indirecting through a pointer doesn't seem to match
> TCTX_AUTO_STORAGE.
>
Indeed. :)
> I guess another case is where there are global variables of the type
> that you want to forbid, compiled while the target feature is enabled,
> and then a function tries to access those variables with the target
> feature locally disabled (through a pragma or attribute). Does that
> case work?
>
Thanks for pointing out this, I tried with the below test case:
__vector_quad a1;
__vector_quad a2;
__attribute__((target("cpu=power8")))
void foo ()
{
a2 = a3;
}
the verify_type_context doesn't catch it as you suspected, I think
it needs some enhancements somewhere.
> That's not an issue for SVE because global variables can't have
> sizeless type.
>
>>> That's not to say we shouldn't have the patch. I'm just not sure
>>> it can be the complete solution.
>>
>> I'm not sure about that either, maybe Andrew have more insights.
>> But as you pointed out in [1], I doubted trying to find all invalid
>> uses of a built-in type is worthwhile, it seems catching those usual
>> cases is enough and practical. So if this verify_type_context
>> framework can cover the most of uses, maybe it's a good direction
>> to go and extend.
>
> IMO it depends on what we're trying to protect against. If the
> compiler can handle these types correctly even when the target feature
> is disabled, and we're simply disallowing the types for policy rather
> than correctness reasons, then maybe just handling the usual cases is
> good enough. But things are different if the compiler is going to ICE
> or generate invalid code when something slips through. In that case,
> I think the niche cases matter too.
>
Thanks for the clarification, good point, I agree! It means we still
need some handlings in movoo and movxo to avoid possible ICE, which can
still be caused by some cases like the above one or similar. This
verify_type_context checking is only a nice add-on to improve the
diagnosis for invalid built-in type. I'm going to fix the expanders,
it should be independent of this patch.
BR,
Kewen
