| Message ID | DUB122-W3250D65A780768771CDEB5E40C0@phx.gbl |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 24, 2013 at 2:55 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote: >> >> On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>>> I think that is common practice to write array[1]; at the end of the >>>> structure, and use it as a flexible array. The compute_record_mode has no >>>> way to decide if that is going to be a flexible array or not. >>>> >>>> Sorry Eric, but now I have no other choice than to go back to my previous >>>> version of part 2: >>>> >>>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html >>> >>> Why? Just set BLKmode in this case as well. >> >> Works for me if it works ABI-wise (which you say it should unless the >> backend is buggy). Note that means mode_for_array should unconditionally >> return BLKmode. > > Did you just propose: > > --- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200 > +++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200 > @@ -471,27 +471,7 @@ > static enum machine_mode > mode_for_array (tree elem_type, tree size) > { > - tree elem_size; > - unsigned HOST_WIDE_INT int_size, int_elem_size; > - bool limit_p; > - > - /* One-element arrays get the component type's mode. */ > - elem_size = TYPE_SIZE (elem_type); > - if (simple_cst_equal (size, elem_size)) > - return TYPE_MODE (elem_type); > - > - limit_p = true; > - if (host_integerp (size, 1) && host_integerp (elem_size, 1)) > - { > - int_size = tree_low_cst (size, 1); > - int_elem_size = tree_low_cst (elem_size, 1); > - if (int_elem_size> 0 > - && int_size % int_elem_size == 0 > - && targetm.array_mode_supported_p (TYPE_MODE (elem_type), > - int_size / int_elem_size)) > - limit_p = false; > - } > - return mode_for_size_tree (size, MODE_INT, limit_p); > + return BLKmode; > } > > ??? Yes. Does it work? Richard. > >> Richard. >> >>> -- >>> Eric Botcazou
On Thu, Oct 24, 2013 at 02:55:59PM +0200, Bernd Edlinger wrote: > On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote: > > > > On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: > >>> I think that is common practice to write array[1]; at the end of the > >>> structure, and use it as a flexible array. The compute_record_mode has no > >>> way to decide if that is going to be a flexible array or not. > >>> > >>> Sorry Eric, but now I have no other choice than to go back to my previous > >>> version of part 2: > >>> > >>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html > >> > >> Why? Just set BLKmode in this case as well. > > > > Works for me if it works ABI-wise (which you say it should unless the > > backend is buggy). Note that means mode_for_array should unconditionally > > return BLKmode. > > Did you just propose: But why would we want to penalize the generated code for this? I mean, if some structure has a flexible array member or something similar to it that we allow to be used as one, if we pass it as arguments, return from functions etc., it will not have any bits beyond those and thus it should be just fine to be passed in registers etc. We can only use those extra bits if either we allocate them on the heap/stack (malloc, alloca, ...) or as a global or automatic variable with initializer that fills in the zero sized array or flexible array member (GNU extensions?), but in either of these cases the DECL_MODE or TYPE_MODE should be irrelevant. So to me this really sounds like a bug in the expansion, certainly not stor-layout. Jakub
>> Did you just propose: >> >> --- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200 >> +++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200 >> @@ -471,27 +471,7 @@ >> static enum machine_mode >> mode_for_array (tree elem_type, tree size) >> { >> - tree elem_size; >> - unsigned HOST_WIDE_INT int_size, int_elem_size; >> - bool limit_p; >> - >> - /* One-element arrays get the component type's mode. */ >> - elem_size = TYPE_SIZE (elem_type); >> - if (simple_cst_equal (size, elem_size)) >> - return TYPE_MODE (elem_type); >> - >> - limit_p = true; >> - if (host_integerp (size, 1) && host_integerp (elem_size, 1)) >> - { >> - int_size = tree_low_cst (size, 1); >> - int_elem_size = tree_low_cst (elem_size, 1); >> - if (int_elem_size> 0 >> - && int_size % int_elem_size == 0 >> - && targetm.array_mode_supported_p (TYPE_MODE (elem_type), >> - int_size / int_elem_size)) >> - limit_p = false; >> - } >> - return mode_for_size_tree (size, MODE_INT, limit_p); >> + return BLKmode; >> } >> >> ??? > > Yes. Does it work? > > Richard. > I will give it a try. I could also explicitly catch the case "struct { a[x]; };" in compute_record_mode. That might work too. But one way or the other that fix will be ugly. Note: struct Y { char c[1]; char c2; }; is "invalid" even if the expander would not fail, because the code in tree-ssa-alias.c will not see the alias, between c[2] and c2, that works only for the last array. So that is no ICE but all generated code at -O1 and higher will be wrong. Bernd.
On Thu, Oct 24, 2013 at 3:13 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Oct 24, 2013 at 02:55:59PM +0200, Bernd Edlinger wrote: >> On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote: >> > >> > On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> >>> I think that is common practice to write array[1]; at the end of the >> >>> structure, and use it as a flexible array. The compute_record_mode has no >> >>> way to decide if that is going to be a flexible array or not. >> >>> >> >>> Sorry Eric, but now I have no other choice than to go back to my previous >> >>> version of part 2: >> >>> >> >>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html >> >> >> >> Why? Just set BLKmode in this case as well. >> > >> > Works for me if it works ABI-wise (which you say it should unless the >> > backend is buggy). Note that means mode_for_array should unconditionally >> > return BLKmode. >> >> Did you just propose: > > But why would we want to penalize the generated code for this? > I mean, if some structure has a flexible array member or something similar > to it that we allow to be used as one, if we pass it as arguments, return > from functions etc., it will not have any bits beyond those and thus it > should be just fine to be passed in registers etc. We can only use those > extra bits if either we allocate them on the heap/stack (malloc, alloca, > ...) or as a global or automatic variable with initializer that fills in the > zero sized array or flexible array member (GNU extensions?), but in either > of these cases the DECL_MODE or TYPE_MODE should be irrelevant. > > So to me this really sounds like a bug in the expansion, certainly not > stor-layout. Sure, that was what I was saying all along ... but still, if we want to fix it in stor-layout.c then we have to fix it for all cases there, not just the zero-size array case (which I showed is not the only relevant case). Richard. > Jakub
Hi Richard, >> Did you just propose: >> >> --- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200 >> +++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200 >> @@ -471,27 +471,7 @@ >> static enum machine_mode >> mode_for_array (tree elem_type, tree size) >> { >> - tree elem_size; >> - unsigned HOST_WIDE_INT int_size, int_elem_size; >> - bool limit_p; >> - >> - /* One-element arrays get the component type's mode. */ >> - elem_size = TYPE_SIZE (elem_type); >> - if (simple_cst_equal (size, elem_size)) >> - return TYPE_MODE (elem_type); >> - >> - limit_p = true; >> - if (host_integerp (size, 1) && host_integerp (elem_size, 1)) >> - { >> - int_size = tree_low_cst (size, 1); >> - int_elem_size = tree_low_cst (elem_size, 1); >> - if (int_elem_size> 0 >> - && int_size % int_elem_size == 0 >> - && targetm.array_mode_supported_p (TYPE_MODE (elem_type), >> - int_size / int_elem_size)) >> - limit_p = false; >> - } >> - return mode_for_size_tree (size, MODE_INT, limit_p); >> + return BLKmode; >> } >> >> ??? > > Yes. Does it work? > > Richard. > No. PASS: gcc.target/i386/pr14289-1.c (test for errors, line 10) FAIL: gcc.target/i386/pr14289-1.c (test for excess errors) Excess errors: /home/ed/gnu/gcc-4.9-20131013/gcc/testsuite/gcc.target/i386/pr14289-1.c:5:14: error: data type of 'a' isn't suitable for a register Does that look like an ABI-Change? the test case uses: register int a[2] asm("ebx"); Bernd.
On Fri, Oct 25, 2013 at 8:29 AM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi Richard, > > >>> Did you just propose: >>> >>> --- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200 >>> +++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200 >>> @@ -471,27 +471,7 @@ >>> static enum machine_mode >>> mode_for_array (tree elem_type, tree size) >>> { >>> - tree elem_size; >>> - unsigned HOST_WIDE_INT int_size, int_elem_size; >>> - bool limit_p; >>> - >>> - /* One-element arrays get the component type's mode. */ >>> - elem_size = TYPE_SIZE (elem_type); >>> - if (simple_cst_equal (size, elem_size)) >>> - return TYPE_MODE (elem_type); >>> - >>> - limit_p = true; >>> - if (host_integerp (size, 1) && host_integerp (elem_size, 1)) >>> - { >>> - int_size = tree_low_cst (size, 1); >>> - int_elem_size = tree_low_cst (elem_size, 1); >>> - if (int_elem_size> 0 >>> - && int_size % int_elem_size == 0 >>> - && targetm.array_mode_supported_p (TYPE_MODE (elem_type), >>> - int_size / int_elem_size)) >>> - limit_p = false; >>> - } >>> - return mode_for_size_tree (size, MODE_INT, limit_p); >>> + return BLKmode; >>> } >>> >>> ??? >> >> Yes. Does it work? >> >> Richard. >> > > No. > > PASS: gcc.target/i386/pr14289-1.c (test for errors, line 10) > FAIL: gcc.target/i386/pr14289-1.c (test for excess errors) > Excess errors: > /home/ed/gnu/gcc-4.9-20131013/gcc/testsuite/gcc.target/i386/pr14289-1.c:5:14: error: data type of 'a' isn't suitable for a register > > Does that look like an ABI-Change? > > the test case uses: > register int a[2] asm("ebx"); Eh ... even register struct { int i; int a[0]; } asm ("ebx"); works. Also with int a[1] but not with a[2]. So just handling trailing arrays makes this case regress as well. Now I'd call this use questionable ... but we've likely supported that for decades and cannot change that now. Back to fixing everything in expand. Richard. > Bernd.
Hi, > Eh ... even > > register struct { int i; int a[0]; } asm ("ebx"); > > works. Also with int a[1] but not with a[2]. So just handling trailing > arrays makes this case regress as well. > > Now I'd call this use questionable ... but we've likely supported that > for decades and cannot change that now. > > Back to fixing everything in expand. > > Richard. > Ok, finally you asked for it. Here is my previous version of that patch again. I have now added a new value "EXPAND_REFERENCE" to the expand_modifier enumeration. It is almost like EXPAND_MEMORY but it does not interfere with constant values. I have done the same modification to VIEW_CONVERT_EXPR too, because this is a possible inner reference, itself. It is however inherently hard to test around this code. To understand this patch it is good to know what type of object the return value "tem" of get_inner_reference can be. From the program logic at get_inner_reference it is clear that the return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably further restricted because exp is gimplified. Usually the result will be a MEM_REF or a SSA_NAME of the memory where the structure is to be found. When you look at where EXPAND_MEMORY is handled you see it is special-cased in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: If it is an unaligned memory, we just return the unaligned reference. This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, because it is only a problem for STRICT_ALIGNMENT targets, and even there it will certainly be really hard to find test cases that exercise this code. In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF we do not have to touch the handling of the outer modifier. However we pass EXPAND_REFERENCE to the inner object, which should not be a recursive use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. Boot-strapped and regression-tested on x86_64-linux-gnu OK for trunk? Thanks Bernd. 2013-10-25 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * expr.h (expand_modifier): New enum value EXPAND_REFERENCE. * expr.c (expand_expr_real_1): Use EXAND_REFERENCE on base object. testsuite: 2013-10-25 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test. * gcc.dg/torture/pr57748-4.c: New test.
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi, > >> Eh ... even >> >> register struct { int i; int a[0]; } asm ("ebx"); >> >> works. Also with int a[1] but not with a[2]. So just handling trailing >> arrays makes this case regress as well. >> >> Now I'd call this use questionable ... but we've likely supported that >> for decades and cannot change that now. >> >> Back to fixing everything in expand. >> >> Richard. >> > > Ok, finally you asked for it. > > Here is my previous version of that patch again. > > I have now added a new value "EXPAND_REFERENCE" to the expand_modifier > enumeration. It is almost like EXPAND_MEMORY but it does not interfere with > constant values. > > I have done the same modification to VIEW_CONVERT_EXPR too, because > this is a possible inner reference, itself. It is however inherently hard to > test around this code. > > To understand this patch it is good to know what type of object the > return value "tem" of get_inner_reference can be. > > From the program logic at get_inner_reference it is clear that the > return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, > ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may > be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably > further restricted because exp is gimplified. > > Usually the result will be a MEM_REF or a SSA_NAME of the memory where > the structure is to be found. > > When you look at where EXPAND_MEMORY is handled you see it is special-cased > in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, > ARRAY_RANGE_REF. > > At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the > same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: > If it is an unaligned memory, we just return the unaligned reference. > > This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, > because it is only a problem for STRICT_ALIGNMENT targets, and even there it will > certainly be really hard to find test cases that exercise this code. > > In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF > we do not have to touch the handling of the outer modifier. However we pass > EXPAND_REFERENCE to the inner object, which should not be a recursive > use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. > > TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle > EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. > > > Boot-strapped and regression-tested on x86_64-linux-gnu > OK for trunk? You point to a weak spot in expansion - that it handles creating the base MEM to offset with handled components by recursing into the case that handles bare MEM_REFs. This makes the bare MEM_REF handling code somewhat awkward (it's the one to assign mem-attrs which are later adjusted for example). Maybe a better appropach than adding yet another expand modifier would be to split out the "base MEM" expansion part out of the bare MEM_REF handling code so we can call that instead of recursing. In this light - instead of a new expand modifier don't you want an actual flag that specifies we are coming from a call that wants to expand a base? That is, allow EXPAND_SUM but with the recursion flag set? Finally I think the recursion into the VIEW_CONVERT_EXPR case is only there because of the keep_aligning flag of get_inner_reference which should be obsolete now that we properly handle its effects in get_object_alignment. So you wouldn't need to adjust this path if we finally can get rid of that. What do others think? Thanks, Richard. > Thanks > Bernd.
Hi, On Fri, Oct 25, 2013 at 12:51:13PM +0200, Richard Biener wrote: > On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: > > Hi, > > > >> Eh ... even > >> > >> register struct { int i; int a[0]; } asm ("ebx"); > >> > >> works. Also with int a[1] but not with a[2]. So just handling trailing > >> arrays makes this case regress as well. > >> > >> Now I'd call this use questionable ... but we've likely supported that > >> for decades and cannot change that now. > >> > >> Back to fixing everything in expand. > >> > >> Richard. > >> > > > > Ok, finally you asked for it. > > > > Here is my previous version of that patch again. > > > > I have now added a new value "EXPAND_REFERENCE" to the expand_modifier > > enumeration. It is almost like EXPAND_MEMORY but it does not interfere with > > constant values. > > > > I have done the same modification to VIEW_CONVERT_EXPR too, because > > this is a possible inner reference, itself. It is however inherently hard to > > test around this code. > > > > To understand this patch it is good to know what type of object the > > return value "tem" of get_inner_reference can be. > > > > From the program logic at get_inner_reference it is clear that the > > return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, > > ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may > > be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably > > further restricted because exp is gimplified. > > > > Usually the result will be a MEM_REF or a SSA_NAME of the memory where > > the structure is to be found. > > > > When you look at where EXPAND_MEMORY is handled you see it is special-cased > > in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, > > ARRAY_RANGE_REF. > > > > At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the > > same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: > > If it is an unaligned memory, we just return the unaligned reference. > > > > This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, > > because it is only a problem for STRICT_ALIGNMENT targets, and even there it will > > certainly be really hard to find test cases that exercise this code. > > > > In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF > > we do not have to touch the handling of the outer modifier. However we pass > > EXPAND_REFERENCE to the inner object, which should not be a recursive > > use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. > > > > TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle > > EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. > > > > > > Boot-strapped and regression-tested on x86_64-linux-gnu > > OK for trunk? > > You point to a weak spot in expansion - that it handles creating > the base MEM to offset with handled components by recursing > into the case that handles bare MEM_REFs. This makes the > bare MEM_REF handling code somewhat awkward (it's the > one to assign mem-attrs which are later adjusted for example). > > Maybe a better appropach than adding yet another expand > modifier would be to split out the "base MEM" expansion part > out of the bare MEM_REF handling code so we can call that > instead of recursing. I think that we should seize every easy opportunity to break up expand_expr* functions into smaller ones with nicer names and easier to understand semantics. So I think is a good idea. Thanks, Martin > > In this light - instead of a new expand modifier don't you want > an actual flag that specifies we are coming from a call that > wants to expand a base? That is, allow EXPAND_SUM > but with the recursion flag set? > > Finally I think the recursion into the VIEW_CONVERT_EXPR case > is only there because of the keep_aligning flag of get_inner_reference > which should be obsolete now that we properly handle its effects > in get_object_alignment. So you wouldn't need to adjust this path > if we finally can get rid of that. > > What do others think? > > Thanks, > Richard. > > > Thanks > > Bernd.
Hi, On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote: > Finally I think the recursion into the VIEW_CONVERT_EXPR case > is only there because of the keep_aligning flag of get_inner_reference > which should be obsolete now that we properly handle its effects > in get_object_alignment. So you wouldn't need to adjust this path > if we finally can get rid of that. I think you are right, this flag is no longer necessary, and removing this code path would simplify everything. Therefore I'd like to propose to remove the "keep_aligning" parameter of get_inner_reference as a split-out patch. Boot-strapped (with languages=all,ada,go) and regression-tested on x86_64-linux-gnu. Ok for trunk? Thanks Bernd. 2013-10-27 Bernd Edlinger <bernd.edlinger@hotmail.de> Remove parameter keep_aligning from get_inner_reference. * tree.h (get_inner_reference): Adjust header. * expr.c (get_inner_reference): Remove parameter keep_aligning. (get_bit_range, expand_assignment, expand_expr_addr_expr_1, expand_expr_real_1): Adjust. * asan.c (instrument_derefs): Adjust. * builtins.c (get_object_alignment_2): Adjust. Remove handling of VIEW_CONVERT_EXPR. * cfgexpand.c (expand_debug_expr): Adjust. * dbxout.c (dbxout_expand_expr): Adjust. * dwarf2out.c (loc_list_for_address_of_addr_expr_of_indirect_ref, loc_list_from_tree, fortran_common): Adjust. * fold-const.c (optimize_bit_field_compare, decode_field_reference, fold_unary_loc, fold_comparison, split_address_to_core_and_offset): Adjust. * gimple-ssa-strength-reduction.c (slsr_process_ref): Adjust. * simplifx-rtx.c (delegitimize_mem_from_attrs): Adjust. * tree-affine.c (tree_to_aff_combination, get_inner_reference_aff): Adjust. * tree-data-ref.c (split_constant_offset_1, dr_analyze_innermost): Adjust. * tree-vect-data-refs.c (vect_check_gather, vect_analyze_data_refs): Adjust. * tree-scalar-evolution.c (interpret_rhs_expr): Adjust. * tree-ssa-loop-ivopts.c (may_be_unaligned_p, split_address_cost): Adjust. * tsan.c (instrument_expr): Adjust. * ada/gcc-interface/decl.c (elaborate_expression_1): Adjust. * ada/gcc-interface/trans.c (Attribute_to_gnu): Adjust. * ada/gcc-interface/utils2.c (build_unary_op): Adjust. * config/mips/mips.c (r10k_safe_mem_expr_p): Adjust.
Hi, On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote: > > On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> Hi, >> >>> Eh ... even >>> >>> register struct { int i; int a[0]; } asm ("ebx"); >>> >>> works. Also with int a[1] but not with a[2]. So just handling trailing >>> arrays makes this case regress as well. >>> >>> Now I'd call this use questionable ... but we've likely supported that >>> for decades and cannot change that now. >>> >>> Back to fixing everything in expand. >>> >>> Richard. >>> >> >> Ok, finally you asked for it. >> >> Here is my previous version of that patch again. >> >> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier >> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with >> constant values. >> >> I have done the same modification to VIEW_CONVERT_EXPR too, because >> this is a possible inner reference, itself. It is however inherently hard to >> test around this code. >> >> To understand this patch it is good to know what type of object the >> return value "tem" of get_inner_reference can be. >> >> From the program logic at get_inner_reference it is clear that the >> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, >> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may >> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably >> further restricted because exp is gimplified. >> >> Usually the result will be a MEM_REF or a SSA_NAME of the memory where >> the structure is to be found. >> >> When you look at where EXPAND_MEMORY is handled you see it is special-cased >> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, >> ARRAY_RANGE_REF. >> >> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the >> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: >> If it is an unaligned memory, we just return the unaligned reference. >> >> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, >> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will >> certainly be really hard to find test cases that exercise this code. >> >> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF >> we do not have to touch the handling of the outer modifier. However we pass >> EXPAND_REFERENCE to the inner object, which should not be a recursive >> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. >> >> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle >> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. >> >> >> Boot-strapped and regression-tested on x86_64-linux-gnu >> OK for trunk? > > You point to a weak spot in expansion - that it handles creating > the base MEM to offset with handled components by recursing > into the case that handles bare MEM_REFs. This makes the > bare MEM_REF handling code somewhat awkward (it's the > one to assign mem-attrs which are later adjusted for example). > > Maybe a better appropach than adding yet another expand > modifier would be to split out the "base MEM" expansion part > out of the bare MEM_REF handling code so we can call that > instead of recursing. > > In this light - instead of a new expand modifier don't you want > an actual flag that specifies we are coming from a call that > wants to expand a base? That is, allow EXPAND_SUM > but with the recursion flag set? > I think you are right. After some thought, I start to like that idea. This way we have at least much more flexibility, how to handle the inner references correctly, and if I change only the interfaces of expand_expr_real/real_1 that will not be used at too many places, either. > Finally I think the recursion into the VIEW_CONVERT_EXPR case > is only there because of the keep_aligning flag of get_inner_reference > which should be obsolete now that we properly handle its effects > in get_object_alignment. So you wouldn't need to adjust this path > if we finally can get rid of that. > I proposed a patch for that, but did not hear from you: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner reference, the code there should be kept in sync with the normal_inner_ref, and MEM_REF, IMHO. Attached, my updated patch for PR57748, Part 2. Boot-strapped and regression-tested on X86_64-pc-linux-gnu. Ok for trunk? Thanks Bernd. > What do others think? > > Thanks, > Richard. > >> Thanks >> Bernd.
oops - this time with attachments... > Hi, > > On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote: >> >> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> Hi, >>> >>>> Eh ... even >>>> >>>> register struct { int i; int a[0]; } asm ("ebx"); >>>> >>>> works. Also with int a[1] but not with a[2]. So just handling trailing >>>> arrays makes this case regress as well. >>>> >>>> Now I'd call this use questionable ... but we've likely supported that >>>> for decades and cannot change that now. >>>> >>>> Back to fixing everything in expand. >>>> >>>> Richard. >>>> >>> >>> Ok, finally you asked for it. >>> >>> Here is my previous version of that patch again. >>> >>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier >>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with >>> constant values. >>> >>> I have done the same modification to VIEW_CONVERT_EXPR too, because >>> this is a possible inner reference, itself. It is however inherently hard to >>> test around this code. >>> >>> To understand this patch it is good to know what type of object the >>> return value "tem" of get_inner_reference can be. >>> >>> From the program logic at get_inner_reference it is clear that the >>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, >>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may >>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably >>> further restricted because exp is gimplified. >>> >>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where >>> the structure is to be found. >>> >>> When you look at where EXPAND_MEMORY is handled you see it is special-cased >>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, >>> ARRAY_RANGE_REF. >>> >>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the >>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: >>> If it is an unaligned memory, we just return the unaligned reference. >>> >>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, >>> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will >>> certainly be really hard to find test cases that exercise this code. >>> >>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF >>> we do not have to touch the handling of the outer modifier. However we pass >>> EXPAND_REFERENCE to the inner object, which should not be a recursive >>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. >>> >>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle >>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. >>> >>> >>> Boot-strapped and regression-tested on x86_64-linux-gnu >>> OK for trunk? >> >> You point to a weak spot in expansion - that it handles creating >> the base MEM to offset with handled components by recursing >> into the case that handles bare MEM_REFs. This makes the >> bare MEM_REF handling code somewhat awkward (it's the >> one to assign mem-attrs which are later adjusted for example). >> >> Maybe a better appropach than adding yet another expand >> modifier would be to split out the "base MEM" expansion part >> out of the bare MEM_REF handling code so we can call that >> instead of recursing. >> >> In this light - instead of a new expand modifier don't you want >> an actual flag that specifies we are coming from a call that >> wants to expand a base? That is, allow EXPAND_SUM >> but with the recursion flag set? >> > > I think you are right. After some thought, I start to like that idea. > > This way we have at least much more flexibility, how to handle the inner > references correctly, and if I change only the interfaces of expand_expr_real/real_1 > that will not be used at too many places, either. > >> Finally I think the recursion into the VIEW_CONVERT_EXPR case >> is only there because of the keep_aligning flag of get_inner_reference >> which should be obsolete now that we properly handle its effects >> in get_object_alignment. So you wouldn't need to adjust this path >> if we finally can get rid of that. >> > > I proposed a patch for that, but did not hear from you: > http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html > > nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner > reference, the code there should be kept in sync with the normal_inner_ref, > and MEM_REF, IMHO. > > Attached, my updated patch for PR57748, Part 2. > > Boot-strapped and regression-tested on X86_64-pc-linux-gnu. > > Ok for trunk? > > > Thanks > Bernd. > >> What do others think? >> >> Thanks, >> Richard. >> >>> Thanks >>> Bernd. 2013-11-07 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * expr.h (expand_expr_real, expand_expr_real_1): Add new parameter expand_reference. (expand_expr, expand_normal): Adjust. * expr.c (expand_expr_real, expand_expr_real_1): Add new parameter expand_reference. Use expand_reference to expand inner references. (store_expr): Adjust. * cfgexpand.c (expand_call_stmt): Adjust. testsuite: 2013-11-07 Bernd Edlinger <bernd.edlinger@hotmail.de> PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test. * gcc.dg/torture/pr57748-4.c: New test.
Hello,
this is a minor update to my previous version of this patch, (using a boolean expand_reference,
instead of adding a new expand_modifier enum value):
I forgot to pass down the expand_reference value at the second expand_expr call inside the
case VIEW_CONVERT_EXPR. Sorry for the inconvenience.
@@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
}
if (!op0)
- op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+ op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+ NULL, expand_reference);
/* If the input and output modes are both the same, we are done. */
if (mode == GET_MODE (op0))
Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
Ok for trunk?
Thanks
Bernd.
> Date: Thu, 7 Nov 2013 13:58:55 +0100
>
> oops - this time with attachments...
>
>
>> Hi,
>>
>> On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
>>>
>>> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
>>> <bernd.edlinger@hotmail.de> wrote:
>>>> Hi,
>>>>
>>>>> Eh ... even
>>>>>
>>>>> register struct { int i; int a[0]; } asm ("ebx");
>>>>>
>>>>> works. Also with int a[1] but not with a[2]. So just handling trailing
>>>>> arrays makes this case regress as well.
>>>>>
>>>>> Now I'd call this use questionable ... but we've likely supported that
>>>>> for decades and cannot change that now.
>>>>>
>>>>> Back to fixing everything in expand.
>>>>>
>>>>> Richard.
>>>>>
>>>>
>>>> Ok, finally you asked for it.
>>>>
>>>> Here is my previous version of that patch again.
>>>>
>>>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier
>>>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
>>>> constant values.
>>>>
>>>> I have done the same modification to VIEW_CONVERT_EXPR too, because
>>>> this is a possible inner reference, itself. It is however inherently hard to
>>>> test around this code.
>>>>
>>>> To understand this patch it is good to know what type of object the
>>>> return value "tem" of get_inner_reference can be.
>>>>
>>>> From the program logic at get_inner_reference it is clear that the
>>>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
>>>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
>>>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
>>>> further restricted because exp is gimplified.
>>>>
>>>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where
>>>> the structure is to be found.
>>>>
>>>> When you look at where EXPAND_MEMORY is handled you see it is special-cased
>>>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
>>>> ARRAY_RANGE_REF.
>>>>
>>>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
>>>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
>>>> If it is an unaligned memory, we just return the unaligned reference.
>>>>
>>>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case,
>>>> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will
>>>> certainly be really hard to find test cases that exercise this code.
>>>>
>>>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
>>>> we do not have to touch the handling of the outer modifier. However we pass
>>>> EXPAND_REFERENCE to the inner object, which should not be a recursive
>>>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
>>>>
>>>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
>>>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
>>>>
>>>>
>>>> Boot-strapped and regression-tested on x86_64-linux-gnu
>>>> OK for trunk?
>>>
>>> You point to a weak spot in expansion - that it handles creating
>>> the base MEM to offset with handled components by recursing
>>> into the case that handles bare MEM_REFs. This makes the
>>> bare MEM_REF handling code somewhat awkward (it's the
>>> one to assign mem-attrs which are later adjusted for example).
>>>
>>> Maybe a better appropach than adding yet another expand
>>> modifier would be to split out the "base MEM" expansion part
>>> out of the bare MEM_REF handling code so we can call that
>>> instead of recursing.
>>>
>>> In this light - instead of a new expand modifier don't you want
>>> an actual flag that specifies we are coming from a call that
>>> wants to expand a base? That is, allow EXPAND_SUM
>>> but with the recursion flag set?
>>>
>>
>> I think you are right. After some thought, I start to like that idea.
>>
>> This way we have at least much more flexibility, how to handle the inner
>> references correctly, and if I change only the interfaces of expand_expr_real/real_1
>> that will not be used at too many places, either.
>>
>>> Finally I think the recursion into the VIEW_CONVERT_EXPR case
>>> is only there because of the keep_aligning flag of get_inner_reference
>>> which should be obsolete now that we properly handle its effects
>>> in get_object_alignment. So you wouldn't need to adjust this path
>>> if we finally can get rid of that.
>>>
>>
>> I proposed a patch for that, but did not hear from you:
>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html
>>
>> nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
>> reference, the code there should be kept in sync with the normal_inner_ref,
>> and MEM_REF, IMHO.
>>
>> Attached, my updated patch for PR57748, Part 2.
>>
>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
>>
>> Ok for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>> What do others think?
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks
>>>> Bernd.
2013-11-07 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR middle-end/57748
* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference.
(expand_expr, expand_normal): Adjust.
* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference. Use expand_reference to expand inner references.
(store_expr): Adjust.
* cfgexpand.c (expand_call_stmt): Adjust.
testsuite:
2013-11-07 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR middle-end/57748
* gcc.dg/torture/pr57748-3.c: New test.
* gcc.dg/torture/pr57748-4.c: New test.
On Sun, Oct 27, 2013 at 5:01 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi, > > On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote: >> Finally I think the recursion into the VIEW_CONVERT_EXPR case >> is only there because of the keep_aligning flag of get_inner_reference >> which should be obsolete now that we properly handle its effects >> in get_object_alignment. So you wouldn't need to adjust this path >> if we finally can get rid of that. > > I think you are right, this flag is no longer necessary, and removing > this code path would simplify everything. Therefore I'd like to propose > to remove the "keep_aligning" parameter of get_inner_reference as > a split-out patch. > > Boot-strapped (with languages=all,ada,go) and > regression-tested on x86_64-linux-gnu. > Ok for trunk? Ok. Thanks, Richard. > Thanks > Bernd.
> I think you are right, this flag is no longer necessary, and removing > this code path would simplify everything. Therefore I'd like to propose > to remove the "keep_aligning" parameter of get_inner_reference as > a split-out patch. > > Boot-strapped (with languages=all,ada,go) and > regression-tested on x86_64-linux-gnu. I don't understand how you can commit a patch that changes something only on strict-alignment platforms and test it only on x86-64. This change *must* be tested with Ada on a strict-alignment platform, that's the only combination for which it is exercised. If you cannot do that, then please back it out. More generally speaking, it's not acceptable to make cleanup changes like that in the RTL expander without extreme care, which of course starts with proper testing. The patch should not have been approved either for that reason.
Hi, On 27 Nov 2013 10:43:59, Eric Botcazou wrote: > >> I think you are right, this flag is no longer necessary, and removing >> this code path would simplify everything. Therefore I'd like to propose >> to remove the "keep_aligning" parameter of get_inner_reference as >> a split-out patch. >> >> Boot-strapped (with languages=all,ada,go) and >> regression-tested on x86_64-linux-gnu. > > I don't understand how you can commit a patch that changes something only on > strict-alignment platforms and test it only on x86-64. This change *must* be > tested with Ada on a strict-alignment platform, that's the only combination Well, I did that. Apologies for not mentioning that. > for which it is exercised. If you cannot do that, then please back it out. > > More generally speaking, it's not acceptable to make cleanup changes like that > in the RTL expander without extreme care, which of course starts with proper > testing. The patch should not have been approved either for that reason. > > -- > Eric Botcazou The change on the ada interface is actually not critical, because all invocations of get_inner_reference there used keep_aligning == false, as did the majority of all other invocations. What changes with that patch, is that get_inner_reference(...., true) could return a VIEW_CONVERT_EXPR, which is now obsolete. If it is causing any trouble, I can revert that change of course. Thanks Bernd.
> Well, I did that. Apologies for not mentioning that. OK, on which strict-alignment platform did you test it with Ada enabled? > The change on the ada interface is actually not critical, because all > invocations of get_inner_reference there used keep_aligning == false, as > did the majority of all other invocations. Sure, but that's not the point... > What changes with that patch, is that get_inner_reference(...., true) could > return a VIEW_CONVERT_EXPR, which is now obsolete. ...that's what needs to be properly verified indeed. > If it is causing any trouble, I can revert that change of course. You might want to take a look at PR middle-end/17746, it took about 3 months almost a decade ago to find something plausible to fix this fundamental issue for Ada on strict-aligmnent platforms so I'd rather not go through this again.
On Wed, Nov 27, 2013 at 10:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> I think you are right, this flag is no longer necessary, and removing >> this code path would simplify everything. Therefore I'd like to propose >> to remove the "keep_aligning" parameter of get_inner_reference as >> a split-out patch. >> >> Boot-strapped (with languages=all,ada,go) and >> regression-tested on x86_64-linux-gnu. > > I don't understand how you can commit a patch that changes something only on > strict-alignment platforms and test it only on x86-64. This change *must* be > tested with Ada on a strict-alignment platform, that's the only combination > for which it is exercised. If you cannot do that, then please back it out. > > More generally speaking, it's not acceptable to make cleanup changes like that > in the RTL expander without extreme care, which of course starts with proper > testing. The patch should not have been approved either for that reason. I'm fine with reverting it for now (you were in CC of the patch submission but silent on it, I asked for the patch to start simplifying the way mems are expanded - ultimately to avoid the recursion and mem-attribute compute by the recursion). We can come back during stage1. get_object_alignment should be able to properly handle this case if you call it on the full reference in the normal_inner_ref: case. All the weird duplicate code on the VIEW_CONVERT_EXPR case should IMHO go. Richard. > -- > Eric Botcazou
On Wed, 27 Nov 2013 12:47:19, Richard Biener wrote: > > On Wed, Nov 27, 2013 at 10:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> I think you are right, this flag is no longer necessary, and removing >>> this code path would simplify everything. Therefore I'd like to propose >>> to remove the "keep_aligning" parameter of get_inner_reference as >>> a split-out patch. >>> >>> Boot-strapped (with languages=all,ada,go) and >>> regression-tested on x86_64-linux-gnu. >> >> I don't understand how you can commit a patch that changes something only on >> strict-alignment platforms and test it only on x86-64. This change *must* be >> tested with Ada on a strict-alignment platform, that's the only combination >> for which it is exercised. If you cannot do that, then please back it out. >> >> More generally speaking, it's not acceptable to make cleanup changes like that >> in the RTL expander without extreme care, which of course starts with proper >> testing. The patch should not have been approved either for that reason. > > I'm fine with reverting it for now (you were in CC of the patch submission > but silent on it, I asked for the patch to start simplifying the way > mems are expanded - ultimately to avoid the recursion and mem-attribute > compute by the recursion). > Ok, then I'll revert this patch in the evening. Bernd. > We can come back during stage1. > > get_object_alignment should be able to properly handle this case > if you call it on the full reference in the normal_inner_ref: case. > All the weird duplicate code on the VIEW_CONVERT_EXPR case > should IMHO go. > > Richard. > >> -- >> Eric Botcazou
> I'm fine with reverting it for now (you were in CC of the patch submission > but silent on it, I asked for the patch to start simplifying the way > mems are expanded - ultimately to avoid the recursion and mem-attribute > compute by the recursion). Because I'm totally lost in this thread and its many sub-threads. > We can come back during stage1. Sure, let's do that instead and not enter stage #3 with hazardous changes. > get_object_alignment should be able to properly handle this case > if you call it on the full reference in the normal_inner_ref: case. How exactly? Once you flatten everything with get_inner_reference at the beginning, the TYPE_ALIGN_OK flag on the VIEW_CONVERT_EXPR is lost. > All the weird duplicate code on the VIEW_CONVERT_EXPR case > should IMHO go. What has changed since 2004 exactly? If you do a grep for TYPE_ALIGN_OK on 4.1 and 4.9 trees, you get exactly the same 4 occurrences in the middle-end.
Hi, ping... this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html Note: it does, as it is, _not_ depend on the keep_aligning patch. And it would fix some really nasty wrong code generation issues. Thanks Bernd. > Date: Tue, 19 Nov 2013 15:39:39 +0100 > > Hello, > > > this is a minor update to my previous version of this patch, (using a boolean expand_reference, > instead of adding a new expand_modifier enum value): > > I forgot to pass down the expand_reference value at the second expand_expr call inside the > case VIEW_CONVERT_EXPR. Sorry for the inconvenience. > > > > @@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac > } > > if (!op0) > - op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier); > + op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier, > + NULL, expand_reference); > > /* If the input and output modes are both the same, we are done. */ > if (mode == GET_MODE (op0)) > > > Boot-strapped and regression-tested on X86_64-pc-linux-gnu. > > Ok for trunk? > > > Thanks > Bernd. > > >> Date: Thu, 7 Nov 2013 13:58:55 +0100 >> >> oops - this time with attachments... >> >> >>> Hi, >>> >>> On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote: >>>> >>>> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger >>>> <bernd.edlinger@hotmail.de> wrote: >>>>> Hi, >>>>> >>>>>> Eh ... even >>>>>> >>>>>> register struct { int i; int a[0]; } asm ("ebx"); >>>>>> >>>>>> works. Also with int a[1] but not with a[2]. So just handling trailing >>>>>> arrays makes this case regress as well. >>>>>> >>>>>> Now I'd call this use questionable ... but we've likely supported that >>>>>> for decades and cannot change that now. >>>>>> >>>>>> Back to fixing everything in expand. >>>>>> >>>>>> Richard. >>>>>> >>>>> >>>>> Ok, finally you asked for it. >>>>> >>>>> Here is my previous version of that patch again. >>>>> >>>>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier >>>>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with >>>>> constant values. >>>>> >>>>> I have done the same modification to VIEW_CONVERT_EXPR too, because >>>>> this is a possible inner reference, itself. It is however inherently hard to >>>>> test around this code. >>>>> >>>>> To understand this patch it is good to know what type of object the >>>>> return value "tem" of get_inner_reference can be. >>>>> >>>>> From the program logic at get_inner_reference it is clear that the >>>>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, >>>>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may >>>>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably >>>>> further restricted because exp is gimplified. >>>>> >>>>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where >>>>> the structure is to be found. >>>>> >>>>> When you look at where EXPAND_MEMORY is handled you see it is special-cased >>>>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, >>>>> ARRAY_RANGE_REF. >>>>> >>>>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the >>>>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: >>>>> If it is an unaligned memory, we just return the unaligned reference. >>>>> >>>>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, >>>>> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will >>>>> certainly be really hard to find test cases that exercise this code. >>>>> >>>>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF >>>>> we do not have to touch the handling of the outer modifier. However we pass >>>>> EXPAND_REFERENCE to the inner object, which should not be a recursive >>>>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. >>>>> >>>>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle >>>>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. >>>>> >>>>> >>>>> Boot-strapped and regression-tested on x86_64-linux-gnu >>>>> OK for trunk? >>>> >>>> You point to a weak spot in expansion - that it handles creating >>>> the base MEM to offset with handled components by recursing >>>> into the case that handles bare MEM_REFs. This makes the >>>> bare MEM_REF handling code somewhat awkward (it's the >>>> one to assign mem-attrs which are later adjusted for example). >>>> >>>> Maybe a better appropach than adding yet another expand >>>> modifier would be to split out the "base MEM" expansion part >>>> out of the bare MEM_REF handling code so we can call that >>>> instead of recursing. >>>> >>>> In this light - instead of a new expand modifier don't you want >>>> an actual flag that specifies we are coming from a call that >>>> wants to expand a base? That is, allow EXPAND_SUM >>>> but with the recursion flag set? >>>> >>> >>> I think you are right. After some thought, I start to like that idea. >>> >>> This way we have at least much more flexibility, how to handle the inner >>> references correctly, and if I change only the interfaces of expand_expr_real/real_1 >>> that will not be used at too many places, either. >>> >>>> Finally I think the recursion into the VIEW_CONVERT_EXPR case >>>> is only there because of the keep_aligning flag of get_inner_reference >>>> which should be obsolete now that we properly handle its effects >>>> in get_object_alignment. So you wouldn't need to adjust this path >>>> if we finally can get rid of that. >>>> >>> >>> I proposed a patch for that, but did not hear from you: >>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html >>> >>> nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner >>> reference, the code there should be kept in sync with the normal_inner_ref, >>> and MEM_REF, IMHO. >>> >>> Attached, my updated patch for PR57748, Part 2. >>> >>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu. >>> >>> Ok for trunk? >>> >>> >>> Thanks >>> Bernd. >>> >>>> What do others think? >>>> >>>> Thanks, >>>> Richard. >>>> >>>>> Thanks >>>>> Bernd.
On Wed, Nov 27, 2013 at 1:29 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> I'm fine with reverting it for now (you were in CC of the patch submission >> but silent on it, I asked for the patch to start simplifying the way >> mems are expanded - ultimately to avoid the recursion and mem-attribute >> compute by the recursion). > > Because I'm totally lost in this thread and its many sub-threads. > >> We can come back during stage1. > > Sure, let's do that instead and not enter stage #3 with hazardous changes. > >> get_object_alignment should be able to properly handle this case >> if you call it on the full reference in the normal_inner_ref: case. > > How exactly? Once you flatten everything with get_inner_reference at the > beginning, the TYPE_ALIGN_OK flag on the VIEW_CONVERT_EXPR is lost. Well, I want tem = get_inner_reference (from, ...); op = expand_expr (tem); ... set_mem_attrs (op, from); instead of setting mem_attrs from 'tem' by some obfuscated means of recursing through multiple levels of expanding the memory reference. That is, completely refactor this stuff as it has become so non-obvious what happens. >> All the weird duplicate code on the VIEW_CONVERT_EXPR case >> should IMHO go. > > What has changed since 2004 exactly? If you do a grep for TYPE_ALIGN_OK on > 4.1 and 4.9 trees, you get exactly the same 4 occurrences in the middle-end. Ah, get_object_alignment used keep_aligning ... Richard. > -- > Eric Botcazou
On Wed, Nov 27, 2013 at 1:29 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > Hi, > > ping... > > this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html > > Note: it does, as it is, _not_ depend on the keep_aligning patch. > And it would fix some really nasty wrong code generation issues. I'll come back to all these patches once the late features have all hit stage3 (hopefully next week). We have some time left to fix this and related bugs. Thanks, Richard. > > Thanks > Bernd. > >> Date: Tue, 19 Nov 2013 15:39:39 +0100 >> >> Hello, >> >> >> this is a minor update to my previous version of this patch, (using a boolean expand_reference, >> instead of adding a new expand_modifier enum value): >> >> I forgot to pass down the expand_reference value at the second expand_expr call inside the >> case VIEW_CONVERT_EXPR. Sorry for the inconvenience. >> >> >> >> @@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac >> } >> >> if (!op0) >> - op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier); >> + op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier, >> + NULL, expand_reference); >> >> /* If the input and output modes are both the same, we are done. */ >> if (mode == GET_MODE (op0)) >> >> >> Boot-strapped and regression-tested on X86_64-pc-linux-gnu. >> >> Ok for trunk? >> >> >> Thanks >> Bernd. >> >> >>> Date: Thu, 7 Nov 2013 13:58:55 +0100 >>> >>> oops - this time with attachments... >>> >>> >>>> Hi, >>>> >>>> On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote: >>>>> >>>>> On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger >>>>> <bernd.edlinger@hotmail.de> wrote: >>>>>> Hi, >>>>>> >>>>>>> Eh ... even >>>>>>> >>>>>>> register struct { int i; int a[0]; } asm ("ebx"); >>>>>>> >>>>>>> works. Also with int a[1] but not with a[2]. So just handling trailing >>>>>>> arrays makes this case regress as well. >>>>>>> >>>>>>> Now I'd call this use questionable ... but we've likely supported that >>>>>>> for decades and cannot change that now. >>>>>>> >>>>>>> Back to fixing everything in expand. >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>> >>>>>> Ok, finally you asked for it. >>>>>> >>>>>> Here is my previous version of that patch again. >>>>>> >>>>>> I have now added a new value "EXPAND_REFERENCE" to the expand_modifier >>>>>> enumeration. It is almost like EXPAND_MEMORY but it does not interfere with >>>>>> constant values. >>>>>> >>>>>> I have done the same modification to VIEW_CONVERT_EXPR too, because >>>>>> this is a possible inner reference, itself. It is however inherently hard to >>>>>> test around this code. >>>>>> >>>>>> To understand this patch it is good to know what type of object the >>>>>> return value "tem" of get_inner_reference can be. >>>>>> >>>>>> From the program logic at get_inner_reference it is clear that the >>>>>> return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, >>>>>> ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may >>>>>> be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably >>>>>> further restricted because exp is gimplified. >>>>>> >>>>>> Usually the result will be a MEM_REF or a SSA_NAME of the memory where >>>>>> the structure is to be found. >>>>>> >>>>>> When you look at where EXPAND_MEMORY is handled you see it is special-cased >>>>>> in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, >>>>>> ARRAY_RANGE_REF. >>>>>> >>>>>> At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the >>>>>> same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: >>>>>> If it is an unaligned memory, we just return the unaligned reference. >>>>>> >>>>>> This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, >>>>>> because it is only a problem for STRICT_ALIGNMENT targets, and even there it will >>>>>> certainly be really hard to find test cases that exercise this code. >>>>>> >>>>>> In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF >>>>>> we do not have to touch the handling of the outer modifier. However we pass >>>>>> EXPAND_REFERENCE to the inner object, which should not be a recursive >>>>>> use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. >>>>>> >>>>>> TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle >>>>>> EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. >>>>>> >>>>>> >>>>>> Boot-strapped and regression-tested on x86_64-linux-gnu >>>>>> OK for trunk? >>>>> >>>>> You point to a weak spot in expansion - that it handles creating >>>>> the base MEM to offset with handled components by recursing >>>>> into the case that handles bare MEM_REFs. This makes the >>>>> bare MEM_REF handling code somewhat awkward (it's the >>>>> one to assign mem-attrs which are later adjusted for example). >>>>> >>>>> Maybe a better appropach than adding yet another expand >>>>> modifier would be to split out the "base MEM" expansion part >>>>> out of the bare MEM_REF handling code so we can call that >>>>> instead of recursing. >>>>> >>>>> In this light - instead of a new expand modifier don't you want >>>>> an actual flag that specifies we are coming from a call that >>>>> wants to expand a base? That is, allow EXPAND_SUM >>>>> but with the recursion flag set? >>>>> >>>> >>>> I think you are right. After some thought, I start to like that idea. >>>> >>>> This way we have at least much more flexibility, how to handle the inner >>>> references correctly, and if I change only the interfaces of expand_expr_real/real_1 >>>> that will not be used at too many places, either. >>>> >>>>> Finally I think the recursion into the VIEW_CONVERT_EXPR case >>>>> is only there because of the keep_aligning flag of get_inner_reference >>>>> which should be obsolete now that we properly handle its effects >>>>> in get_object_alignment. So you wouldn't need to adjust this path >>>>> if we finally can get rid of that. >>>>> >>>> >>>> I proposed a patch for that, but did not hear from you: >>>> http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html >>>> >>>> nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner >>>> reference, the code there should be kept in sync with the normal_inner_ref, >>>> and MEM_REF, IMHO. >>>> >>>> Attached, my updated patch for PR57748, Part 2. >>>> >>>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu. >>>> >>>> Ok for trunk? >>>> >>>> >>>> Thanks >>>> Bernd. >>>> >>>>> What do others think? >>>>> >>>>> Thanks, >>>>> Richard. >>>>> >>>>>> Thanks >>>>>> Bernd.
On 11/27/13 05:29, Bernd Edlinger wrote: > Hi, > > ping... > > this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html > > Note: it does, as it is, _not_ depend on the keep_aligning patch. > And it would fix some really nasty wrong code generation issues. Is there a testcase for this problem? My recommendation is to start a separate thread which focuses on this issue and only this issue. jeff
Hi, On Wed, 27 Nov 2013 12:07:16, Jeff Law wrote: > > On 11/27/13 05:29, Bernd Edlinger wrote: >> Hi, >> >> ping... >> >> this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html >> >> Note: it does, as it is, _not_ depend on the keep_aligning patch. >> And it would fix some really nasty wrong code generation issues. > Is there a testcase for this problem? Yes, the patch contains two test cases, one for struct S { V a; V b[0]; } P __attribute__((aligned (1))) and another for struct S { V b[1]; } P __attribute__((aligned (1))) V can be anything that has a movmisalign_optab or is SLOW_UNALIGNED_ACCESS If V::b is used as flexible array, reading p->b[1] gives garbage. We tried hard, to fix this in stor-layout.c by not using the mode of V for struct S, but this created ABI-fallout. So currently the only possible way to fix it seems to be in expansion, by letting expand_real_1 know that we need a memory reference, even if it may be unaligned. > > My recommendation is to start a separate thread which focuses on this > issue and only this issue. > If there are more questions of general interest, please feel free to start in a new thread. > jeff > Thanks Bernd.
On 11/28/13 03:24, Bernd Edlinger wrote: > Hi, > > On Wed, 27 Nov 2013 12:07:16, Jeff Law wrote: >> >> On 11/27/13 05:29, Bernd Edlinger wrote: >>> Hi, >>> >>> ping... >>> >>> this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html >>> >>> Note: it does, as it is, _not_ depend on the keep_aligning patch. >>> And it would fix some really nasty wrong code generation issues. >> Is there a testcase for this problem? > > Yes, > the patch contains two test cases, one for > > struct S { V a; V b[0]; } P __attribute__((aligned (1))) > and another for > > struct S { V b[1]; } P __attribute__((aligned (1))) > > > V can be anything that has a movmisalign_optab or is SLOW_UNALIGNED_ACCESS > > If V::b is used as flexible array, reading p->b[1] gives garbage. > > We tried hard, to fix this in stor-layout.c by not using the mode of V > for struct S, but this created ABI-fallout. So currently the only possible > way to fix it seems to be in expansion, by letting expand_real_1 know that > we need a memory reference, even if it may be unaligned. > >> >> My recommendation is to start a separate thread which focuses on this >> issue and only this issue. >> > > If there are more questions of general interest, please feel free to start > in a new thread. Well, my point is there's this thread that deals with multiple issues; the message with the patch itself references prior versions of the patch and sorting out any discussion that relates strictly to the outstanding patch is nontrivial. Hence my request to repost the patch as a new independent thread. Include the testcase and a reference to any current bugzilla bugs. jeff
Hi, On Wed, 27 Nov 2013 12:47:19, Richard Biener wrote: > > On Wed, Nov 27, 2013 at 10:43 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: >>> I think you are right, this flag is no longer necessary, and removing >>> this code path would simplify everything. Therefore I'd like to propose >>> to remove the "keep_aligning" parameter of get_inner_reference as >>> a split-out patch. >>> >>> Boot-strapped (with languages=all,ada,go) and >>> regression-tested on x86_64-linux-gnu. >> >> I don't understand how you can commit a patch that changes something only on >> strict-alignment platforms and test it only on x86-64. This change *must* be >> tested with Ada on a strict-alignment platform, that's the only combination >> for which it is exercised. If you cannot do that, then please back it out. >> >> More generally speaking, it's not acceptable to make cleanup changes like that >> in the RTL expander without extreme care, which of course starts with proper >> testing. The patch should not have been approved either for that reason. > > I'm fine with reverting it for now (you were in CC of the patch submission > but silent on it, I asked for the patch to start simplifying the way > mems are expanded - ultimately to avoid the recursion and mem-attribute > compute by the recursion). > > We can come back during stage1. > Well, it's stage1 again. I still have that already-approved patch, updated to current trunk. I've successfully boot-strapped it on armv7-linux-gnueabihf with all languages enabled, including Ada. The test suite runs cleanly without any drop-outs. Is it OK to commit now, or are there objections? Thanks Bernd. > get_object_alignment should be able to properly handle this case > if you call it on the full reference in the normal_inner_ref: case. > All the weird duplicate code on the VIEW_CONVERT_EXPR case > should IMHO go. > > Richard. > >> -- >> Eric Botcazou 2014-04-16 Bernd Edlinger <bernd.edlinger@hotmail.de> Remove parameter keep_aligning from get_inner_reference. * tree.h (get_inner_reference): Adjust header. * expr.c (get_inner_reference): Remove parameter keep_aligning. (get_bit_range, expand_assignment, expand_expr_addr_expr_1, expand_expr_real_1): Adjust. * asan.c (instrument_derefs): Adjust. * builtins.c (get_object_alignment_2): Adjust. Remove handling of VIEW_CONVERT_EXPR. * cfgexpand.c (expand_debug_expr): Adjust. * dbxout.c (dbxout_expand_expr): Adjust. * dwarf2out.c (loc_list_for_address_of_addr_expr_of_indirect_ref, loc_list_from_tree, fortran_common): Adjust. * fold-const.c (optimize_bit_field_compare, decode_field_reference, fold_unary_loc, fold_comparison, split_address_to_core_and_offset): Adjust. * gimple-ssa-strength-reduction.c (slsr_process_ref): Adjust. * simplifx-rtx.c (delegitimize_mem_from_attrs): Adjust. * tree-affine.c (tree_to_aff_combination, get_inner_reference_aff): Adjust. * tree-data-ref.c (split_constant_offset_1, dr_analyze_innermost): Adjust. * tree-vect-data-refs.c (vect_check_gather, vect_analyze_data_refs): Adjust. * tree-scalar-evolution.c (interpret_rhs_expr): Adjust. * tree-ssa-loop-ivopts.c ( split_address_cost): Adjust. * tsan.c (instrument_expr): Adjust. * config/mips/mips.c (r10k_safe_mem_expr_p): Adjust. ada: 2014-04-16 Bernd Edlinger <bernd.edlinger@hotmail.de> Remove parameter keep_aligning from get_inner_reference. * gcc-interface/decl.c (elaborate_expression_1): Adjust. * gcc-interface/trans.c (Attribute_to_gnu): Adjust. * gcc-interface/utils2.c (build_unary_op): Adjust.
> I still have that already-approved patch, updated to current trunk. > I've successfully boot-strapped it on armv7-linux-gnueabihf with > all languages enabled, including Ada. > The test suite runs cleanly without any drop-outs. Thanks for the testing. > Is it OK to commit now, or are there objections? I think that the patch is either incomplete or wrong, in the sense that it will break TYPE_ALIGN_OK support, unless this support is totally obsolete, in which case it ought to be totally removed instead of just partially. The Ada testsuite in the compiler isn't exhaustive enough to give any guarantee so I will need to conduct more testing. Can you sit on the patch a few weeks?
Hi Eric, On Tue, 22 Apr 2014 10:09:28, Eric Botcazou wrote: > >> I still have that already-approved patch, updated to current trunk. >> I've successfully boot-strapped it on armv7-linux-gnueabihf with >> all languages enabled, including Ada. >> The test suite runs cleanly without any drop-outs. > > Thanks for the testing. > >> Is it OK to commit now, or are there objections? > > I think that the patch is either incomplete or wrong, in the sense that it > will break TYPE_ALIGN_OK support, unless this support is totally obsolete, in > which case it ought to be totally removed instead of just partially. The Ada > testsuite in the compiler isn't exhaustive enough to give any guarantee so I > will need to conduct more testing. Can you sit on the patch a few weeks? > Sure, and thanks again for your help. I was not able to find any difference on the generated code with or without that patch. Bernd. > -- > Eric Botcazou
> Sure, and thanks again for your help. Thanks! > I was not able to find any difference on the generated code with > or without that patch. Yes, my gut feeling is that TYPE_ALIGN_OK is really obsolete now. It is set in a single place in the compiler (gcc-interface/decl.c:gnat_to_gnu_entity): /* Tell the middle-end that objects of tagged types are guaranteed to be properly aligned. This is necessary because conversions to the class-wide type are translated into conversions to the root type, which can be less aligned than some of its derived types. */ if (Is_Tagged_Type (gnat_entity) || Is_Class_Wide_Equivalent_Type (gnat_entity)) TYPE_ALIGN_OK (gnu_type) = 1; but we changed the way these conversions are done some time ago.
On 04/22/14 03:25, Eric Botcazou wrote: >> Sure, and thanks again for your help. > > Thanks! > >> I was not able to find any difference on the generated code with >> or without that patch. > > Yes, my gut feeling is that TYPE_ALIGN_OK is really obsolete now. It is set > in a single place in the compiler (gcc-interface/decl.c:gnat_to_gnu_entity): > > /* Tell the middle-end that objects of tagged types are guaranteed to > be properly aligned. This is necessary because conversions to the > class-wide type are translated into conversions to the root type, > which can be less aligned than some of its derived types. */ > if (Is_Tagged_Type (gnat_entity) > || Is_Class_Wide_Equivalent_Type (gnat_entity)) > TYPE_ALIGN_OK (gnu_type) = 1; > > but we changed the way these conversions are done some time ago. So does this remove the last concern around Bernd's patch? Jeff
On Fri, May 2, 2014 at 8:18 AM, Jeff Law <law@redhat.com> wrote: > On 04/22/14 03:25, Eric Botcazou wrote: >>> >>> Sure, and thanks again for your help. >> >> >> Thanks! >> >>> I was not able to find any difference on the generated code with >>> or without that patch. >> >> >> Yes, my gut feeling is that TYPE_ALIGN_OK is really obsolete now. It is >> set >> in a single place in the compiler >> (gcc-interface/decl.c:gnat_to_gnu_entity): >> >> /* Tell the middle-end that objects of tagged types are guaranteed >> to >> be properly aligned. This is necessary because conversions to >> the >> class-wide type are translated into conversions to the root type, >> which can be less aligned than some of its derived types. */ >> if (Is_Tagged_Type (gnat_entity) >> || Is_Class_Wide_Equivalent_Type (gnat_entity)) >> TYPE_ALIGN_OK (gnu_type) = 1; >> >> but we changed the way these conversions are done some time ago. > > So does this remove the last concern around Bernd's patch? And can we remove TYPE_ALIGN_OK as followup? (ISTR it's used by obj-c/c++ as well, but I can't find such use) Thanks, Richard. > Jeff >
On May 5, 2014, at 1:00 AM, Richard Biener <richard.guenther@gmail.com> wrote: > And can we remove TYPE_ALIGN_OK as followup? (ISTR it's used > by obj-c/c++ as well, but I can't find such use) I didn’t find any current hint… the only landmine I found was: ./gcc/config/pa/pa.c:1878: /* Using TYPE_ALIGN_OK is rather conservative as ./gcc/config/pa/pa.c:1880: align = (TYPE_ALIGN_OK (type) ? TYPE_ALIGN (type)
> > So does this remove the last concern around Bernd's patch? > > And can we remove TYPE_ALIGN_OK as followup? (ISTR it's used > by obj-c/c++ as well, but I can't find such use) Probably but, as previously indicated, I need to do some testing first.
Hi Eric, On Wed, 14 May 2014 09:28:55, Eric Botcazou wrote: > >>> So does this remove the last concern around Bernd's patch? >> >> And can we remove TYPE_ALIGN_OK as followup? (ISTR it's used >> by obj-c/c++ as well, but I can't find such use) > > Probably but, as previously indicated, I need to do some testing first. > > -- > Eric Botcazou Ok sure, I'll be patient... If I remove this line, the build fails: gcc-interface/decl.c:gnat_to_gnu_entity: /* Tell the middle-end that objects of tagged types are guaranteed to be properly aligned. This is necessary because conversions to the class-wide type are translated into conversions to the root type, which can be less aligned than some of its derived types. */ if (Is_Tagged_Type (gnat_entity) || Is_Class_Wide_Equivalent_Type (gnat_entity)) TYPE_ALIGN_OK (gnu_type) = 1; but only because this bit is read back in the ada/gcc-interface. If I apply my patch, and additionally remove this line in expr.c, which is one of the last references to TYPE_ALIGN_OK in the middle-end: if (TYPE_ALIGN_OK (type)) { /* ??? Copying the MEM without substantially changing it might run afoul of the code handling volatile memory references in store_expr, which assumes that TARGET is returned unmodified if it has been used. */ op0 = copy_rtx (op0); set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type))); } nothing changes. All test cases pass, and again I can not see any difference in the generated code (I compare gcc/ada/*.o with and without patch). So, currently I am under the impression that TYPE_ALIGN_OK has some relevance to the Ada front-end, but it should not be used in the middle-end and certainly not in the back-end. Bernd.
--- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200 +++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200 @@ -471,27 +471,7 @@ static enum machine_mode mode_for_array (tree elem_type, tree size) { - tree elem_size; - unsigned HOST_WIDE_INT int_size, int_elem_size; - bool limit_p; - - /* One-element arrays get the component type's mode. */ - elem_size = TYPE_SIZE (elem_type); - if (simple_cst_equal (size, elem_size)) - return TYPE_MODE (elem_type); - - limit_p = true; - if (host_integerp (size, 1) && host_integerp (elem_size, 1)) - { - int_size = tree_low_cst (size, 1); - int_elem_size = tree_low_cst (elem_size, 1); - if (int_elem_size> 0 - && int_size % int_elem_size == 0 - && targetm.array_mode_supported_p (TYPE_MODE (elem_type), - int_size / int_elem_size)) - limit_p = false; - } - return mode_for_size_tree (size, MODE_INT, limit_p); + return BLKmode; } ???