| Message ID | 20151019194102.GA48581@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
> this is patch that reverts the TYPE_MODE mismatch related changes and > adds test to type checker that TYPE_MODE always match with TYPE_CANONICAL. > I have bootstrapped/regtested x86_64-linux, but unfrtunately the regtesting > had some unrelated noise (spawn failures). I am re-testing. I am on a trip > and will likely only access interenet again from Des Moines tonight. Thanks! > > * tree.c (verify_type): Verify that TYPE_MODE match > between TYPE_CANONICAL and type. > * expr.c (store_expr_with_bounds): Revert my previous change. > * expmed.c (store_bit_field_1): Revert prevoius change. > * gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE > to match for complete types. Please add the PR middle-end/67966 reference to the ChangeLog.
> > > > * tree.c (verify_type): Verify that TYPE_MODE match > > between TYPE_CANONICAL and type. > > * expr.c (store_expr_with_bounds): Revert my previous change. > > * expmed.c (store_bit_field_1): Revert prevoius change. > > * gimple-expr.c (useless_type_conversion_p): Require TYPE_MODE > > to match for complete types. > > Please add the PR middle-end/67966 reference to the ChangeLog. Added and comitted now. Honza > > -- > Eric Botcazou
+ /* Changes in machine mode are never useless conversions unless. */ Unless what? Andreas.
> 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?
* gnat.dg/discr44.adb: New test.
> > 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? 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;
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). 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; >
> > 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). Yep, you have varargs and K&R prototypes, so it can't work this way. > > 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). I guess gain is really limited to Ada - there are very few cases we do VCE otherwise. (I think we could do more of them). We can make useless_type_conversion NOP/CONVERT only. That in fact makes quite a sense because those are types with gimple operations on it. Perhaps also VCE on vectors, but not VCE in general. Honza > > 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). > > 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; > >
On Thu, Oct 29, 2015 at 4:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> 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). > > Yep, you have varargs and K&R prototypes, so it can't work this way. Well, then I suppose we need to compute the ABI upfront when we gimplify from the orginal args (like we preserve fntype). Having a separate fntype was really meant to make us preserve the ABI throughout the GIMPLE phase... >> >> 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). > > I guess gain is really limited to Ada - there are very few cases we do VCE otherwise. > (I think we could do more of them). We can make useless_type_conversion NOP/CONVERT > only. That in fact makes quite a sense because those are types with gimple operations > on it. Perhaps also VCE on vectors, but not VCE in general. > > Honza >> >> 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). >> >> 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; >> >
> On Thu, Oct 29, 2015 at 4:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > >> > >> 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). > > > > Yep, you have varargs and K&R prototypes, so it can't work this way. > > Well, then I suppose we need to compute the ABI upfront when we gimplify > from the orginal args (like we preserve fntype). Having a separate fntype > was really meant to make us preserve the ABI throughout the GIMPLE phase... Hmm, the idea of doing some part of ABI explicitly is definitly nice (at least the implicit promotions and pass by reference bits), but storing the full lowlevel info on how to pass argument seems bit steep. You will need to preserve the RTL containers for parameters that may get non-trivial (PARALLEL) and precompute all the other information how to get data on stack. While playing with the ABi checker I was just looking into this after several years (when i was cleaning up calls.c) and calls.c basically works by computing arg_data that holds most of the info you would need (you need also return argument passing and the hidden argument for structure returns). You can check it out - it is fairly non-trivial beast plus it really holds two parallel sets of infos - tailcall and normal call (because these differ for targets with register windows). The info also depends on flags used to compile function body (such as -maccumulate-outgoing-args) To make something like this a permanent part of GIMPLE would probably need quite careful re-engineering of the APIs inventing more high-level intermediate representation to get out of the machine description. There is not realy immediate benefit from knowing how parameters are housed on stack for gimple optimizers, so perhaps just keeping the type information (after promotions) as the way to specify call conventions is more practical way to go. Honza > >> 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). > > > > I guess gain is really limited to Ada - there are very few cases we do VCE otherwise. > > (I think we could do more of them). We can make useless_type_conversion NOP/CONVERT > > only. That in fact makes quite a sense because those are types with gimple operations > > on it. Perhaps also VCE on vectors, but not VCE in general. > > > > Honza > >> > >> 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). > >> > >> 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; > >> >
On Thu, Oct 29, 2015 at 4:52 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> On Thu, Oct 29, 2015 at 4:02 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> >> >> 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). >> > >> > Yep, you have varargs and K&R prototypes, so it can't work this way. >> >> Well, then I suppose we need to compute the ABI upfront when we gimplify >> from the orginal args (like we preserve fntype). Having a separate fntype >> was really meant to make us preserve the ABI throughout the GIMPLE phase... > > Hmm, the idea of doing some part of ABI explicitly is definitly nice (at least > the implicit promotions and pass by reference bits), but storing the full > lowlevel info on how to pass argument seems bit steep. You will need to > preserve the RTL containers for parameters that may get non-trivial (PARALLEL) > and precompute all the other information how to get data on stack. > > While playing with the ABi checker I was just looking into this after several > years (when i was cleaning up calls.c) and calls.c basically works by computing > arg_data that holds most of the info you would need (you need also return > argument passing and the hidden argument for structure returns). You can check > it out - it is fairly non-trivial beast plus it really holds two parallel sets > of infos - tailcall and normal call (because these differ for targets with > register windows). The info also depends on flags used to compile function body > (such as -maccumulate-outgoing-args) > > To make something like this a permanent part of GIMPLE would probably need quite > careful re-engineering of the APIs inventing more high-level intermediate > representation to get out of the machine description. There is not realy immediate > benefit from knowing how parameters are housed on stack for gimple optimizers, so > perhaps just keeping the type information (after promotions) as the way to specify > call conventions is more practical way to go. Yeah, I suppose we'd need to either build a new function type for each variadic call then or somehow represent 'fntype' differently (note that function attributes also need to be preserved). Richard. > Honza > >> >> 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). >> > >> > I guess gain is really limited to Ada - there are very few cases we do VCE otherwise. >> > (I think we could do more of them). We can make useless_type_conversion NOP/CONVERT >> > only. That in fact makes quite a sense because those are types with gimple operations >> > on it. Perhaps also VCE on vectors, but not VCE in general. >> > >> > Honza >> >> >> >> 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). >> >> >> >> 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; >> >> >
> > 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). > > I guess gain is really limited to Ada - there are very few cases we do VCE > otherwise. (I think we could do more of them). We can make > useless_type_conversion NOP/CONVERT only. That in fact makes quite a sense > because those are types with gimple operations on it. Perhaps also VCE on > vectors, but not VCE in general. FWIW that's fine with me. Yes, Ada tends to generate a lot of VCEs but I try to get rid of the useless ones as much as I can so assistance from the middle- end is not really required. I'll test Richard's patch and install it if the outcome is positive (unless you want to do the vector thing right away).
> > > 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). > > > > I guess gain is really limited to Ada - there are very few cases we do VCE > > otherwise. (I think we could do more of them). We can make > > useless_type_conversion NOP/CONVERT only. That in fact makes quite a sense > > because those are types with gimple operations on it. Perhaps also VCE on > > vectors, but not VCE in general. > > FWIW that's fine with me. Yes, Ada tends to generate a lot of VCEs but I try > to get rid of the useless ones as much as I can so assistance from the middle- > end is not really required. I'll test Richard's patch and install it if the > outcome is positive (unless you want to do the vector thing right away). Lets go with this patch and hopefully stabilize the tree. I don't think the vector conversions represent an important case. Honza > > -- > Eric Botcazou
> > Yeah, I suppose we'd need to either build a new function type for each > variadic call > then or somehow represent 'fntype' differently (note that function > attributes also > need to be preserved). Why we can't keep fntype as it is, but simply add a new set of parameters to call stmt that lists their types? We can then feed those types to expand_call (since we still go back to generic here I suppose we will just re-insert those VCEs in expand-cfg.c) Honza
> Lets go with this patch and hopefully stabilize the tree. I don't think the > vector conversions represent an important case. Unfortunately the patch introduces GIMPLE checking failures in Ada so it will need to be completed/improved. But let's postpone it because we have another class of GIMPLE checking failures introduced by the useless_type_conversion_p change itself: c37213j.adb:21:05: warning: variable "X" is read but never assigned c37213j.adb: In function 'C37213J.PROC.CONSTPROP': c37213j.adb:41:4: error: invalid conversion in gimple call struct c37213j__proc__value___PAD struct c37213j__proc__value___PAD # .MEM_38 = VDEF <.MEM_37> MEM[(struct c37213j__proc__value___PAD *)R.12_25] = c37213j.proc.value (); [static-chain: &FRAME.39] [return slot optimization] and: eric@polaris:~/build/gcc/native> ~/install/gnat-head/bin/gcc -S c37213j.adb - O2 c37213j.adb:21:05: warning: variable "X" is read but never assigned c37213j.adb: In function 'C37213J.PROC.VALUE': c37213j.adb:26:5: error: invalid conversion in return statement struct c37213j__proc__value___PAD struct c37213j__proc__value___PAD # VUSE <.MEM_11> return _9(D); What happens here is that GIMPLE statements are remapped through cloning and we have a variably-modified type returned by a nested function, so the type of the LHS of a GIMPLE_CALL or that of the RHS of a GIMPLE_RETURN is remapped but of course not the return type of the function. This used to be OK because remapping is done by means of copy_node and preserves TYPE_CANONICAL, so the conversion between remapped and original type was deemed useless; now the TYPE_CANONICAL check is gone so the conversion is not useless anymore... I don't think that we want to introduce an artificial VCE to fix this so we probably need a couple of kludges in the GIMPLE verifier instead. In any case, the more I look into the fallout of the useless_type_conversion_p change, the more I find it ill-advised. We used to have a solid type system in the middle-end by means of the predicate and now we have cases for which it ought to return false and returns true (e.g. non-structurally equivalent types with different calling conventions) and cases for which it can return true and returns false (remapped types or types deemed equivalent by the languages). I don't really know what it was made for, but there must be a better way... * gnat.dg/discr45.adb: New test.
On October 31, 2015 6:17:35 PM GMT+01:00, Eric Botcazou <ebotcazou@adacore.com> wrote: >> Lets go with this patch and hopefully stabilize the tree. I don't >think the >> vector conversions represent an important case. > >Unfortunately the patch introduces GIMPLE checking failures in Ada so >it will >need to be completed/improved. But let's postpone it because we have >another >class of GIMPLE checking failures introduced by the >useless_type_conversion_p >change itself: > >c37213j.adb:21:05: warning: variable "X" is read but never assigned >c37213j.adb: In function 'C37213J.PROC.CONSTPROP': >c37213j.adb:41:4: error: invalid conversion in gimple call >struct c37213j__proc__value___PAD > >struct c37213j__proc__value___PAD > ># .MEM_38 = VDEF <.MEM_37> >MEM[(struct c37213j__proc__value___PAD *)R.12_25] = c37213j.proc.value >(); >[static-chain: &FRAME.39] [return slot optimization] > >and: > >eric@polaris:~/build/gcc/native> ~/install/gnat-head/bin/gcc -S >c37213j.adb - >O2 >c37213j.adb:21:05: warning: variable "X" is read but never assigned >c37213j.adb: In function 'C37213J.PROC.VALUE': >c37213j.adb:26:5: error: invalid conversion in return statement >struct c37213j__proc__value___PAD > >struct c37213j__proc__value___PAD > ># VUSE <.MEM_11> >return _9(D); > >What happens here is that GIMPLE statements are remapped through >cloning and >we have a variably-modified type returned by a nested function, so the >type of >the LHS of a GIMPLE_CALL or that of the RHS of a GIMPLE_RETURN is >remapped but >of course not the return type of the function. This used to be OK >because >remapping is done by means of copy_node and preserves TYPE_CANONICAL, >so the >conversion between remapped and original type was deemed useless; now >the >TYPE_CANONICAL check is gone so the conversion is not useless >anymore... > >I don't think that we want to introduce an artificial VCE to fix this >so we >probably need a couple of kludges in the GIMPLE verifier instead. > >In any case, the more I look into the fallout of the >useless_type_conversion_p >change, the more I find it ill-advised. We used to have a solid type >system >in the middle-end by means of the predicate and now we have cases for >which it >ought to return false and returns true (e.g. non-structurally >equivalent types >with different calling conventions) and cases for which it can return >true and >returns false (remapped types or types deemed equivalent by the >languages). >I don't really know what it was made for, but there must be a better >way... I suggest to re-instantiate the canonical type checks for the aggregate type case. And add all these test cases. Richard. > > * gnat.dg/discr45.adb: New test.
Eric Botcazou <ebotcazou@adacore.com> writes:
> * gnat.dg/discr45.adb: New test.
This fails on ia64.
raised CONSTRAINT_ERROR : discr45.adb:19 range check failed
#0 <__gnat_rcheck_CE_Range_Check> (file=(system.address) 0x4000000000022e98,
line=19) at a-except.adb:1286
#1 0x4000000000010470 in discr45.proc (signal=true)
at /usr/local/gcc/gcc-20151102/gcc/testsuite/gnat.dg/discr45.adb:19
#2 0x40000000000104a0 in discr45 ()
at /usr/local/gcc/gcc-20151102/gcc/testsuite/gnat.dg/discr45.adb:43
Andreas.
On Fri, Oct 30, 2015 at 4:19 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> >> Yeah, I suppose we'd need to either build a new function type for each >> variadic call >> then or somehow represent 'fntype' differently (note that function >> attributes also >> need to be preserved). > > Why we can't keep fntype as it is, but simply add a new set of parameters to call stmt > that lists their types? We can then feed those types to expand_call (since we still go > back to generic here I suppose we will just re-insert those VCEs in expand-cfg.c) That would be quite expensive though ... (the extra list of parameters). > Honza
> This fails on ia64.
gnat.dg/discr44.adb and gnat.dg/discr45.adb are supposed to fail everywhere
(and there are also a few ACATS failures everywhere). Sorry for the awkward
situation but they are testcases exposing the recent type system breakage.
> > This fails on ia64. > > gnat.dg/discr44.adb and gnat.dg/discr45.adb are supposed to fail everywhere > (and there are also a few ACATS failures everywhere). Sorry for the awkward > situation but they are testcases exposing the recent type system breakage. Are these supposed to be fixed by Richard's change to not use useless_type_conversion for VCE, or is it another issue? Honza > > -- > Eric Botcazou
> Are these supposed to be fixed by Richard's change to not use > useless_type_conversion for VCE, or is it another issue? Richard's change not to use useless_type_conversion for VCE was causing additional GIMPLE verification failures so I didn't pursue; I can try again, but all the known regressions are now fixed thanks to Richard's latest change to useless_type_conversion_p itself.
> > Are these supposed to be fixed by Richard's change to not use > > useless_type_conversion for VCE, or is it another issue? > > Richard's change not to use useless_type_conversion for VCE was causing > additional GIMPLE verification failures so I didn't pursue; I can try again, > but all the known regressions are now fixed thanks to Richard's latest change > to useless_type_conversion_p itself. I see, you re-instantiated the TYPE_CANONICAL check for aggregates instead. I guess it is most practical way to go right now even though it would be really nice to separate this from TBAA machinery. At the moment LTO doesn't do globbing where calling conventions should care. One such case is the globing of array containing char and char which is required by Fortran standard, but that IMO is a defect in standard - if types are passed differently by target ABI one can't expect them to be fuly interoperable as Fortran would like. Thank you very much for looking into this! Honza > > -- > Eric Botcazou
On Wed, Nov 4, 2015 at 5:50 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> > Are these supposed to be fixed by Richard's change to not use >> > useless_type_conversion for VCE, or is it another issue? >> >> Richard's change not to use useless_type_conversion for VCE was causing >> additional GIMPLE verification failures so I didn't pursue; I can try again, >> but all the known regressions are now fixed thanks to Richard's latest change >> to useless_type_conversion_p itself. > > I see, you re-instantiated the TYPE_CANONICAL check for aggregates instead. I > guess it is most practical way to go right now even though it would be really nice > to separate this from TBAA machinery. > At the moment LTO doesn't do globbing where calling conventions should care. > One such case is the globing of array containing char and char which is required > by Fortran standard, but that IMO is a defect in standard - if types are passed > differently by target ABI one can't expect them to be fuly interoperable as Fortran > would like. Note that I can't see how non-register type defs/uses will ever "change" their type during optimization so I think using TYPE_CANONICAL for the aggregate type case is fine. Richard. > Thank you very much for looking into this! > Honza >> >> -- >> Eric Botcazou
Index: tree.c =================================================================== --- tree.c (revision 228933) +++ tree.c (working copy) @@ -13344,6 +13344,14 @@ verify_type (const_tree t) error_found = true; } + if (COMPLETE_TYPE_P (t) && TYPE_CANONICAL (t) + && TYPE_MODE (t) != TYPE_MODE (TYPE_CANONICAL (t))) + { + error ("TYPE_MODE of TYPE_CANONICAL is not compatible"); + debug_tree (ct); + error_found = true; + } + /* Check various uses of TYPE_MINVAL. */ if (RECORD_OR_UNION_TYPE_P (t)) Index: expr.c =================================================================== --- expr.c (revision 228933) +++ expr.c (working copy) @@ -5425,14 +5425,6 @@ store_expr_with_bounds (tree exp, rtx ta temp = convert_modes (GET_MODE (target), TYPE_MODE (TREE_TYPE (exp)), temp, TYPE_UNSIGNED (TREE_TYPE (exp))); - /* We allow move between structures of same size but different mode. - If source is in memory and the mode differs, simply change the memory. */ - if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode) - { - gcc_assert (MEM_P (temp)); - temp = adjust_address_nv (temp, GET_MODE (target), 0); - } - /* If value was not generated in the target, store it there. Convert the value to TARGET's type first if necessary and emit the pending incrementations that have been queued when expanding EXP. Index: expmed.c =================================================================== --- expmed.c (revision 228933) +++ expmed.c (working copy) @@ -757,14 +757,6 @@ store_bit_field_1 (rtx str_rtx, unsigned } } - /* We allow move between structures of same size but different mode. - If source is in memory and the mode differs, simply change the memory. */ - if (GET_MODE (value) == BLKmode && GET_MODE (op0) != BLKmode) - { - gcc_assert (MEM_P (value)); - value = adjust_address_nv (value, GET_MODE (op0), 0); - } - /* Storing an lsb-aligned field in a register can be done with a movstrict instruction. */ Index: gimple-expr.c =================================================================== --- gimple-expr.c (revision 228933) +++ gimple-expr.c (working copy) @@ -88,9 +88,10 @@ useless_type_conversion_p (tree outer_ty return true; /* Changes in machine mode are never useless conversions unless we - deal with aggregate types in which case we defer to later checks. */ + deal with complete aggregate types in which case we defer to later + checks. */ if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type) - && !AGGREGATE_TYPE_P (inner_type)) + && (!AGGREGATE_TYPE_P (inner_type) || COMPLETE_TYPE_P (outer_type))) return false; /* If both the inner and outer types are integral types, then the