Re: Add VIEW_CONVERT_EXPR to operand_equal_p

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Oct 29, 2015 at 12:22 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Oct 29, 2015 at 4:39 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>> > Added and comitted now.
>>>
>>> Thanks.  Now on to the wrong code issues. :-)
>>>
>>> Up to the change, the useless_type_conversion_p predicate was relying on
>>> structural equivalence via the TYPE_CANONICAL check, now it only looks at the
>>> outermost level (size, mode).  Now some back-ends, most notably x86-64, do a
>>> deep structural scan to determine the calling conventions (classify_argument)
>>> instead of just looking at the size and the mode, so consistency dictates that
>>> the type of the argument and that of the parameter be structurally equivalent
>>> and this sometimes can only be achieved by a VCE... which is now deleted. :-(
>>> See the call to derivedIP in the attached testcase which now fails on x86-64.
>>>
>>> How do we get away from here?
>>
>> Hmm, I noticed this in ipa-icf context and wrote checker that two functions are ABI
>> compatile (did not pushed it out yet), but of course this is nastier.
>>
>> I think the problem exists before my patch with LTO - it is just matter of
>> doing two types which will be considered equivalent by
>> gimple_canonical_types_compatible_p but have different type conversion.  An
>> example of such type would be:
>>
>> struct a {
>>   int a[4];
>> };
>> struct b {
>>   int a[4];
>> } __attribute__ ((__aligned__(16)));
>>
>> I tried to turn this into an testcase, the problem is that I don't know of a way
>> to obtain VIEW_CONVERT_EXPR between the two types out of C or C++ frontend and we
>> don't seem to synthetize these in middle end (even in cases it would make sense).
>> I will try to play with it more - would be nice to have a C reproducer.
>>
>> We may be safe before my patch from wrong code issues if there is no way to
>> rpduce VIEW_CONVERT_EXPR between types like this in languages that support
>> aligned attribute.
>>
>> I think the problem is generally similar to memory references - the gimple type
>> compatibility should not be tied to ABI details.  Probably most consistent
>> solution would be to extend GIMPLE_CALL to also list types of parameters and do
>> not rely on whatever type the operand have....
>>
>> Richard, any ideas?
>
> IMHO it was always wrong/fragile for backends to look at the actual arguments to
> decide on the calling convention.  The backends should _solely_ rely on
> gimple_call_fntype and its TYPE_ARG_TYPES here.
>
> Of course then there are varargs ... (not sure if we hit this here).
>
> But yes, the VIEW_CONVERT "stripping" is a bit fragile and I don't remember
> what exactly we gain from it (when not done on registers).
>
> But I also don't see where we do the stripping mentioned on memory references.
> The match.pd pattern doesn't apply to memory, only in the GENERIC path
> which is guarded with exact type equality.  So I can't see where we end up
> stripping the V_C_E.
>
> There is one bogus case still in fold-const.c:
>
>     case VIEW_CONVERT_EXPR:
>       if (TREE_CODE (op0) == MEM_REF)
>         /* ???  Bogus for aligned types.  */
>         return fold_build2_loc (loc, MEM_REF, type,
>                                 TREE_OPERAND (op0, 0), TREE_OPERAND (op0, 1));
>
>       return NULL_TREE;
>
> that comment is only in my local tree ... (we lose alignment info that is
> on the original MEM_REF type which may be a smaller one).

Ah - tree_ssa_useless_type_conversion and callers, during gimplification.
I'd like to get rid of it but maybe simply delete the VIEW_CONVERT_EXPR
case from it for now (and return true unconditionally for NON_LVALUE_EXPR).

Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c      (revision 229517)
+++ gcc/tree-ssa.c      (working copy)
@@ -1142,13 +1161,16 @@ delete_tree_ssa (struct function *fn)
 bool
 tree_ssa_useless_type_conversion (tree expr)
 {
+  /* Not strictly a conversion but this function is used to strip
+     useless stuff from trees returned from GENERIC folding.  */
+  if (TREE_CODE (expr) == NON_LVALUE_EXPR)
+    return true;
+
   /* If we have an assignment that merely uses a NOP_EXPR to change
      the top of the RHS to the type of the LHS and the type conversion
      is "safe", then strip away the type conversion so that we can
      enter LHS = RHS into the const_and_copies table.  */
-  if (CONVERT_EXPR_P (expr)
-      || TREE_CODE (expr) == VIEW_CONVERT_EXPR
-      || TREE_CODE (expr) == NON_LVALUE_EXPR)
+  if (CONVERT_EXPR_P (expr))
     return useless_type_conversion_p
       (TREE_TYPE (expr),
        TREE_TYPE (TREE_OPERAND (expr, 0)));

IMHO the gimplifier use should be more explicit and most remaining GIMPLE
middle-end uses should be removed (after auditing).

Richard.

> Richard.
>
>> Honza
>>>
>>>
>>>       * gnat.dg/discr44.adb: New test.
>>>
>>> --
>>> Eric Botcazou
>>
>>> -- { dg-do run }
>>> -- { dg-options "-gnatws" }
>>>
>>> procedure Discr44 is
>>>
>>>   function Ident (I : Integer) return Integer is
>>>   begin
>>>     return I;
>>>   end;
>>>
>>>   type Int is range 1 .. 10;
>>>
>>>   type Str is array (Int range <>) of Character;
>>>
>>>   type Parent (D1, D2 : Int; B : Boolean) is record
>>>     S : Str (D1 .. D2);
>>>   end record;
>>>
>>>   type Derived (D : Int) is new Parent (D1 => D, D2 => D, B => False);
>>>
>>>   X1 : Derived (D => Int (Ident (7)));
>>>
>>> begin
>>>   if X1.D /= 7 then
>>>     raise Program_Error;
>>>   end if;
>>> end;
>>