Re: [PATCH] Fix aarch64 PR 99657: ICE with SVE types used without an error

["Kewen.Lin" <linkw@linux.ibm.com>]

on 2022/12/7 20:55, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi Richard,
>>
>> on 2022/12/7 17:16, Richard Sandiford wrote:
>>> "Kewen.Lin" <linkw@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