Re: [PATCH v2] Handle OPAQUE_TYPE specially in verify_type [PR106833]

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

on 2022/9/9 15:25, Richard Biener wrote:
> On Fri, Sep 9, 2022 at 8:51 AM Kewen.Lin <linkw@linux.ibm.com> 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 <linkw@linux.ibm.com>:
>>>>
>>>> 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