[PATCH] Remove "keep_aligning" from get_inner_reference
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 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. patch-inner-reference.diff Description: Binary data
RE: [PATCH] Remove "keep_aligning" from get_inner_reference
Hi, On Wed, 27 Nov 2013 12:47:19, Richard Biener wrote: > > On Wed, Nov 27, 2013 at 10:43 AM, 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 >> 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 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 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. patch-inner-reference.diff Description: Binary data
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> 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? -- Eric Botcazou
RE: [PATCH] Remove "keep_aligning" from get_inner_reference
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
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> 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. -- Eric Botcazou
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
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
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
On Fri, May 2, 2014 at 8:18 AM, Jeff Law 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 >
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
On May 5, 2014, at 1:00 AM, Richard Biener 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)
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> > 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
RE: [PATCH] Remove "keep_aligning" from get_inner_reference
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.
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
On Sun, Oct 27, 2013 at 5:01 PM, Bernd Edlinger 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.
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> 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. -- Eric Botcazou
RE: [PATCH] Remove "keep_aligning" from get_inner_reference
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.
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> 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. -- Eric Botcazou
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
On Wed, Nov 27, 2013 at 10:43 AM, 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 > 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
RE: [PATCH] Remove "keep_aligning" from get_inner_reference
On Wed, 27 Nov 2013 12:47:19, Richard Biener wrote: > > On Wed, Nov 27, 2013 at 10:43 AM, 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 >> 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
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> 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. -- Eric Botcazou
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
On Wed, Nov 27, 2013 at 1:29 PM, Eric Botcazou 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
Re: [PATCH] Remove "keep_aligning" from get_inner_reference
> Ah, get_object_alignment used keep_aligning ...
Yes, the patch contains the rather explicit hunks:
Index: gcc/builtins.c
===
--- gcc/builtins.c (revision 204101)
+++ gcc/builtins.c (working copy)
@@ -315,7 +315,7 @@ get_object_alignment_2 (tree exp, unsigned int *al
/* Get the innermost object and the constant (bitpos) and possibly
variable (offset) offset of the access. */
exp = get_inner_reference (exp, &bitsize, &bitpos, &offset,
-&mode, &unsignedp, &volatilep, true);
+&mode, &unsignedp, &volatilep);
/* Extract alignment information from the innermost object and
possibly adjust bitpos and offset. */
@@ -346,10 +346,6 @@ get_object_alignment_2 (tree exp, unsigned int *al
align = DECL_ALIGN (exp);
known_alignment = true;
}
- else if (TREE_CODE (exp) == VIEW_CONVERT_EXPR)
-{
- align = TYPE_ALIGN (TREE_TYPE (exp));
-}
else if (TREE_CODE (exp) == INDIRECT_REF
|| TREE_CODE (exp) == MEM_REF
|| TREE_CODE (exp) == TARGET_MEM_REF)
--
Eric Botcazou
