| Message ID | CAFiYyc3WuxWAS+s3FjB+BGz1bXhxAxM88S-Fj+Gsn0T6DOS6Mg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 29, 2015 at 12:31 PM, Richard Biener <richard.guenther@gmail.com> wrote: > 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). The above is pre-approved if one of you does the required testing (it fixes the Ada testcase for me). Richard. > 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; >>>
> 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))); The patch introduces GIMPLE checking failures: FAIL: c52103m FAIL: c52103r FAIL: c52104m FAIL: c52104r of the form: slice9.adb: In function 'Slice9': slice9.adb:1:1: error: conversion of an SSA_NAME on the left hand side VIEW_CONVERT_EXPR<character[D.4195:iftmp.10]>("ABCDE"); D.4379 = &VIEW_CONVERT_EXPR<character[D.4195:iftmp.10]>("ABCDE")[D.4378 ...] {lb: D.4195 sz: 1}; if (TREE_CODE (expr) == VIEW_CONVERT_EXPR) { /* For VIEW_CONVERT_EXPRs which are allowed here too, we only check that their operand is not an SSA name or an invariant when requiring an lvalue (this usually means there is a SRA or IPA-SRA bug). Otherwise there is nothing to verify, gross mismatches at most invoke undefined behavior. */ if (require_lvalue && (TREE_CODE (op) == SSA_NAME || is_gimple_min_invariant (op))) { error ("conversion of an SSA_NAME on the left hand side"); debug_generic_stmt (expr); return true; } else if (TREE_CODE (op) == SSA_NAME && TYPE_SIZE (TREE_TYPE (expr)) != TYPE_SIZE (TREE_TYPE (op))) { error ("conversion of register to a different size"); debug_generic_stmt (expr); return true; } else if (!handled_component_p (op)) return false; } It's related to dynamic slicing (reduced testcase attached). * gnat.dg/slice9.adb: New test.
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)));