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. :)

Tested as before.

Does this updated patch look good to you?

BR,
Kewen
------