Re: [PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]
On 9/9/22 8:47 PM, Segher Boessenkool wrote: > On Fri, Sep 09, 2022 at 07:56:42PM -0500, Peter Bergner wrote: >> On 9/9/22 8:27 AM, Kewen.Lin wrote: >>> btw, it needs some hacking in rs6000_function_arg to make this >>> opaque type valid for function arg. >> >> We don't allow (at this time) __vector_pair or __vector_quad to be >> used as actual arguments to non-builtin functions. We do allow >> pointers to those types though. > > It would be nice to support that, if it isn't too hard. It won't be > digging us into a hole, experience has taught us :-) Sure, but we didn't need it at the time (and still don't) and if we do add support for that, we'll have to update the ABIs to describe how they are passed and returned and that is no small feat in itself. It's just a fair amount of work when no one is actually asking for that support and we have a lot of other things to work on. Peter
Re: [PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]
On Fri, Sep 09, 2022 at 07:56:42PM -0500, Peter Bergner wrote: > On 9/9/22 8:27 AM, Kewen.Lin wrote: > > btw, it needs some hacking in rs6000_function_arg to make this > > opaque type valid for function arg. > > We don't allow (at this time) __vector_pair or __vector_quad to be > used as actual arguments to non-builtin functions. We do allow > pointers to those types though. It would be nice to support that, if it isn't too hard. It won't be digging us into a hole, experience has taught us :-) Segher
Re: [PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]
On 9/9/22 8:27 AM, Kewen.Lin wrote:
> __attribute__((noipa))
> int foo(c){
> return 0;
> }
>
> int main ()
> {
> const __vector_quad c;
> int r = foo(c);
> return r;
> }
>
> Checking during LTO WPA, verify_type only gets type "const
> __vector_quad", no type "__vector_quad".
>
> btw, it needs some hacking in rs6000_function_arg to make this
> opaque type valid for function arg.
We don't allow (at this time) __vector_pair or __vector_quad to be
used as actual arguments to non-builtin functions. We do allow
pointers to those types though.
Peter
Re: [PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]
on 2022/9/9 15:25, Richard Biener wrote:
> On Fri, Sep 9, 2022 at 8:51 AM Kewen.Lin wrote:
>>
>> Hi Richi,
>>
>> Thanks for the review comments!
>>
>> on 2022/9/8 15:36, Richard Biener wrote:
>>>
>>>
Am 08.09.2022 um 07:53 schrieb Kewen.Lin :
Hi,
As PR106833 shows, cv-qualified opaque type can cause ICE
during LTO. It exposes that we missd to handle OPAQUE_TYPE
well in type verification. As Richi pointed out, also
assuming that target will always define TYPE_MAIN_VARIANT
and TYPE_CANONICAL for opaque type, this patch is to check
both are OPAQUE_TYPE_P. Besides, it also checks the only
available size and alignment information as well as type
mode for TYPE_MAIN_VARIANT.
>> ...
+
+ if (t != tv)
+{
+ verify_match (TREE_CODE, t, tv);
+ verify_match (TYPE_MODE, t, tv);
+ verify_match (TYPE_SIZE, t, tv);
>>>
>>> TYPE_SIZE is a tree, you should probably
>>> Compare this with operand_equal_p. It’s
>>> Not documented to be a constant size?
>>> Thus some VLA vector mode might be allowed ( a poly_int size),
>>
>> Thanks for catching, I was referencing the code in function
>> verify_type_variant, that corresponding part seems imperfect:
>>
>> if (TREE_CODE (TYPE_SIZE (t)) != PLACEHOLDER_EXPR
>> && TREE_CODE (TYPE_SIZE (tv)) != PLACEHOLDER_EXPR)
>> verify_variant_match (TYPE_SIZE);
>>
>> I agree poly_int size is allowed, the patch was updated for it.
>>
>> BLKmode
>>> Is ruled out(?),
>>
>> Yes, it requires a mode of MODE_OPAQUE class.
>>
>> the docs say we have
>>> ‚An MODE_Opaque‘ here but I don’t see
>>> This being verified?
>>>
>>
>> There is a MODE equality check, I assumed the given t already
>> has one MODE_OPAQUE mode, but the patch was updated to make
>> it explicit as you concerned.
>>
>>> The macro makes this a bit unworldly
>>> For the only benefit of elaborate diagnostic
>>> Which I think isn’t really necessary
>>
>> OK, fixed!
>>
>> The previous version makes just one check on TYPE_CANONICAL to
>> be cheap as gimple_canonical_types_compatible_p said, but
>> since there are just several fields to be check, this updated
>> version adjusted it to be the same as what's for TYPE_MAIN_VARIANT.
>> Hope it's fine. :)
>
> I think we'll call verify_type on the main variant as well so that would be
> redundant (ensured by transitivity), can you check?
I just had a check and found that we don't always call verify_type
on the main variant. For example, with one case like:
__attribute__((noipa))
int foo(c){
return 0;
}
int main ()
{
const __vector_quad c;
int r = foo(c);
return r;
}
Checking during LTO WPA, verify_type only gets type "const
__vector_quad", no type "__vector_quad".
btw, it needs some hacking in rs6000_function_arg to make this
opaque type valid for function arg.
>
>> Tested as before.
>>
>> Does this updated patch look good to you?
>
> Yes, please remove the checks against the main variant if the above holds,
> OK with or without that change depending on this outcome.
>
Committed in r13-2562, thanks!
BR,
Kewen
Re: [PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]
On Fri, Sep 9, 2022 at 8:51 AM Kewen.Lin wrote:
>
> Hi Richi,
>
> Thanks for the review comments!
>
> on 2022/9/8 15:36, Richard Biener wrote:
> >
> >
> >> Am 08.09.2022 um 07:53 schrieb Kewen.Lin :
> >>
> >> Hi,
> >>
> >> As PR106833 shows, cv-qualified opaque type can cause ICE
> >> during LTO. It exposes that we missd to handle OPAQUE_TYPE
> >> well in type verification. As Richi pointed out, also
> >> assuming that target will always define TYPE_MAIN_VARIANT
> >> and TYPE_CANONICAL for opaque type, this patch is to check
> >> both are OPAQUE_TYPE_P. Besides, it also checks the only
> >> available size and alignment information as well as type
> >> mode for TYPE_MAIN_VARIANT.
> >>
> ...
> >> +
> >> + if (t != tv)
> >> +{
> >> + verify_match (TREE_CODE, t, tv);
> >> + verify_match (TYPE_MODE, t, tv);
> >> + verify_match (TYPE_SIZE, t, tv);
> >
> > TYPE_SIZE is a tree, you should probably
> > Compare this with operand_equal_p. It’s
> > Not documented to be a constant size?
> > Thus some VLA vector mode might be allowed ( a poly_int size),
>
> Thanks for catching, I was referencing the code in function
> verify_type_variant, that corresponding part seems imperfect:
>
> if (TREE_CODE (TYPE_SIZE (t)) != PLACEHOLDER_EXPR
> && TREE_CODE (TYPE_SIZE (tv)) != PLACEHOLDER_EXPR)
> verify_variant_match (TYPE_SIZE);
>
> I agree poly_int size is allowed, the patch was updated for it.
>
> BLKmode
> > Is ruled out(?),
>
> Yes, it requires a mode of MODE_OPAQUE class.
>
> the docs say we have
> > ‚An MODE_Opaque‘ here but I don’t see
> > This being verified?
> >
>
> There is a MODE equality check, I assumed the given t already
> has one MODE_OPAQUE mode, but the patch was updated to make
> it explicit as you concerned.
>
> > The macro makes this a bit unworldly
> > For the only benefit of elaborate diagnostic
> > Which I think isn’t really necessary
>
> OK, fixed!
>
> The previous version makes just one check on TYPE_CANONICAL to
> be cheap as gimple_canonical_types_compatible_p said, but
> since there are just several fields to be check, this updated
> version adjusted it to be the same as what's for TYPE_MAIN_VARIANT.
> Hope it's fine. :)
I think we'll call verify_type on the main variant as well so that would be
redundant (ensured by transitivity), can you check?
> Tested as before.
>
> Does this updated patch look good to you?
Yes, please remove the checks against the main variant if the above holds,
OK with or without that change depending on this outcome.
Thanks,
Richard.
>
> BR,
> Kewen
> --
