Re: [patch 4/6] scalar-storage-order merge: bulk

[Jeff Law <law at redhat dot com>]

On 10/06/2015 05:04 AM, Eric Botcazou wrote:
> This is the bulk of the implementation.
>
> 	* calls.c (store_unaligned_arguments_into_pseudos): Adjust calls to
> 	extract_bit_field and store_bit_field.
> 	(initialize_argument_information): Adjust call to store_expr.
> 	(load_register_parameters): Adjust call to extract_bit_field.
> 	* expmed.c (check_reverse_storage_order_support): New function.
> 	(check_reverse_float_storage_order_support): Likewise.
> 	(flip_storage_order): Likewise.
> 	(store_bit_field_1): Add REVERSE parameter.  Flip the storage order
> 	of the value if it is true.  Pass REVERSE to recursive call after
> 	adjusting the target offset.
> 	Do not use extraction or movstrict instruction if REVERSE is true.
> 	Pass REVERSE to store_fixed_bit_field.
> 	(store_bit_field): Add REVERSE parameter and pass to it to above.
> 	(store_fixed_bit_field): Add REVERSE parameter and pass to it to
> 	store_split_bit_field and store_fixed_bit_field_1.
> 	(store_fixed_bit_field_1):  Add REVERSE parameter.  Flip the storage
> 	order of the value if it is true and adjust the target offset.
> 	(store_split_bit_field): Add REVERSE parameter and pass it to
> 	store_fixed_bit_field.  Adjust the target offset if it is true.
> 	(extract_bit_field_1): Add REVERSE parameter.  Flip the storage order
> 	of the value if it is true.  Pass REVERSE to recursive call after
> 	adjusting the target offset.
> 	Do not use extraction or subreg instruction if REVERSE is true.
> 	Pass REVERSE to extract_fixed_bit_field.
> 	(extract_bit_field): Add REVERSE parameter and pass to it to above.
> 	(extract_fixed_bit_field): Add REVERSE parameter and pass to it to
> 	extract_split_bit_field and extract_fixed_bit_field_1.
> 	(extract_fixed_bit_field_1): Add REVERSE parameter.  Flip the storage
> 	order of the value if it is true and adjust the target offset.
> 	(extract_split_bit_field): Add REVERSE parameter and pass it to
> 	extract_fixed_bit_field.  Adjust the target offset if it is true.
> 	* expmed.h (flip_storage_order): Declare.
> 	(store_bit_field): Adjust prototype.
> 	(extract_bit_field): Likewise.
> 	* expr.c (emit_group_load_1): Adjust calls to extract_bit_field.
> 	(emit_group_store): Adjust call to store_bit_field.
> 	(copy_blkmode_from_reg): Likewise.
> 	(copy_blkmode_to_reg): Likewise.
> 	(write_complex_part): Likewise.
>   	(read_complex_part): Likewise.
> 	(optimize_bitfield_assignment_op): Add REVERSE parameter.  Assert
> 	that it isn't true if the target is a register.
> 	<PLUS_EXPR>: If it is, do not optimize unless bitsize is equal to 1,
> 	and flip the storage order of the value.
> 	<BIT_IOR_EXPR>: Flip the storage order of the value.
> 	(get_bit_range): Adjust call to get_inner_reference.
> 	(expand_assignment): Adjust calls to get_inner_reference, store_expr,
> 	optimize_bitfield_assignment_op and store_field.  Handle MEM_EXPRs
> 	with reverse storage order.
> 	(store_expr_with_bounds): Add REVERSE parameter and pass it to
> 	recursive calls and call to store_bit_field.  Force the value into a
> 	register if it is true and then flip the storage order of the value.
> 	(store_expr): Add REVERSE parameter and pass it to above.
> 	(categorize_ctor_elements_1): Adjust call to
> 	initializer_constant_valid_p.
> 	(store_constructor_field): Add REVERSE parameter and pass it to
> 	recursive calls and call to store_field.
> 	(store_constructor): Add REVERSE parameter and pass it to calls to
> 	store_constructor_field and store_expr.  Set it to true for an
> 	aggregate type with TYPE_REVERSE_STORAGE_ORDER.
> 	(store_field): Add REVERSE parameter and pass it to recursive calls
> 	and calls to store_expr and store_bit_field.  Temporarily flip the
> 	storage order of the value with record type and integral mode and
> 	adjust the shift if it is true.
> 	(get_inner_reference): Add PREVERSEP parameter and set it to true
> 	upon encoutering a reference with reverse storage order.
> 	(expand_expr_addr_expr_1): Adjust call to get_inner_reference.
> 	(expand_constructor): Adjust call to store_constructor.
> 	(expand_expr_real_2) <CASE_CONVERT>: Pass TYPE_REVERSE_STORAGE_ORDER
> 	of the union type to store_expr in the MEM case and assert that it
> 	isn't set in the REG case.  Adjust call to store_field.
> 	(expand_expr_real_1) <MEM_REF>: Handle reverse storage order.
> 	<normal_inner_ref>: Add REVERSEP variable and adjust calls to
> 	get_inner_reference and extract_bit_field. Temporarily flip the
> 	storage order of the value with record type and integral mode and
> 	adjust the shift if it is true.  Flip the storage order of the value
> 	at the end if it is true.
> 	<VIEW_CONVERT_EXPR>: Add REVERSEP variable and adjust call to
> 	get_inner_reference.  Do not fetch an inner reference if it is true.
> 	* expr.h (store_expr_with_bounds): Ajust prototype.
> 	(store_expr): Likewise.
> 	* fold-const.c (make_bit_field_ref): Add REVERSEP parameter and set
> 	REF_REVERSE_STORAGE_ORDER on the reference according to it.
> 	(optimize_bit_field_compare): Deal with reverse storage order.
> 	Adjust calls to get_inner_reference and make_bit_field_ref.
> 	(decode_field_reference): Add PREVERSEP parameter and adjust call to
> 	get_inner_reference.
> 	(fold_truth_andor_1): Deal with reverse storage order.  Adjust calls
> 	to decode_field_reference and make_bit_field_ref.
> 	(fold_unary_loc) <CASE_CONVERT>: Adjust call to get_inner_reference.
> 	<VIEW_CONVERT_EXPR>: Propagate the REF_REVERSE_STORAGE_ORDER flag.
> 	(fold_comparison): Adjust call to get_inner_reference.
> 	(split_address_to_core_and_offset): Adjust call to
> 	get_inner_reference.
> 	* gimple-expr.c (useless_type_conversion_p): Return false for array
> 	types with different TYPE_REVERSE_STORAGE_ORDER flag.
> 	* gimplify.c (gimplify_expr) <MEM_REF>: Propagate the
> 	REF_REVERSE_STORAGE_ORDER flag.
> 	* lto-streamer-out.c (hash_tree): Deal with
> 	TYPE_REVERSE_STORAGE_ORDER.
> 	* output.h (assemble_real): Adjust prototype.
> 	* print-tree.c (print_node): Convey TYPE_REVERSE_STORAGE_ORDER.
> 	* stor-layout.c (finish_record_layout): Propagate the
> 	TYPE_REVERSE_STORAGE_ORDER flag to the variants.
> 	* tree-core.h (TYPE_REVERSE_STORAGE_ORDER): Document.
> 	(TYPE_SATURATING): Adjust.
> 	(REF_REVERSE_STORAGE_ORDER): Document.
> 	* tree-dfa.c (get_ref_base_and_extent): Add PREVERSE parameter and
> 	set it to true upon encoutering a reference with reverse storage
> 	order.
> 	* tree-dfa.h (get_ref_base_and_extent): Adjust prototype.
> 	* tree-inline.c (remap_gimple_op_r): Propagate the
> 	REF_REVERSE_STORAGE_ORDER flag.
> 	(copy_tree_body_r): Likewise.
> 	* tree-outof-ssa.c (insert_value_copy_on_edge): Adjust call to
> 	store_expr.
> 	* tree-streamer-in.c (unpack_ts_base_value_fields): Deal with
> 	TYPE_REVERSE_STORAGE_ORDER and REF_REVERSE_STORAGE_ORDER.
> 	* tree-streamer-out.c (pack_ts_base_value_fields): Likewise.
> 	* tree.c (stabilize_reference) <BIT_FIELD_REF>: Propagate the
> 	REF_REVERSE_STORAGE_ORDER flag.
> 	(verify_type_variant): Deal with TYPE_REVERSE_STORAGE_ORDER.
> 	(gimple_canonical_types_compatible_p): Likewise.
> 	* tree.h (TYPE_REVERSE_STORAGE_ORDER): New flag.
> 	(TYPE_SATURATING): Adjust.
> 	(REF_REVERSE_STORAGE_ORDER): New flag.
> 	(reverse_storage_order_for_component_p): New inline predicate.
>   	(storage_order_barrier_p): Likewise.
> 	(get_inner_reference): Adjust prototype.
> 	* varasm.c (assemble_real): Add REVERSE parameter.  Flip the storage
> 	order of the value if REVERSE is true.
> 	(compare_constant) <CONSTRUCTOR>: Compare TYPE_REVERSE_STORAGE_ORDER.
> 	(assemble_constant_contents): Adjust call to output_constant.
> 	(output_constant_pool_2): Adjust call to assemble_real.
> 	(initializer_constant_valid_p_1) <CONSTRUCTOR>: Deal with
> 	TYPE_REVERSE_STORAGE_ORDER.
> 	(output_constant): Add REVERSE parameter.
> 	<INTEGER_TYPE>: Flip the storage order of the value if REVERSE is
> 	true.
> 	<REAL_TYPE>: Adjust call to assemble_real.
> 	<COMPLEX_TYPE>: Pass it to recursive calls.
> 	<ARRAY_TYPE>: Likewise.  Adjust call to output_constructor.
> 	<RECORD_TYPE>: Likewise.  Adjust call to output_constructor.
> 	(struct oc_local_state): Add REVERSE field.
> 	(output_constructor_array_range): Adjust calls to output_constant.
> 	(output_constructor_regular_field): Likewise.
> 	(output_constructor_bitfield): Adjust call to output_constructor.
> 	Flip the storage order of the value if REVERSE is true.
> 	(output_constructor): Add REVERSE parameter.  Set it to true for an
> 	aggregate type with TYPE_REVERSE_STORAGE_ORDER.  Adjust call to
> 	output_constructor_bitfield.
> lto/
> 	* lto.c (compare_tree_sccs_1): Deal with TYPE_REVERSE_STORAGE_ORDER.
I must admit, I'm surprised at how many places we compare types in 
ever-so-slightly different ways.  This kind of patch makes that obvious. 
 Not asking you to fix that, just a minor rant.

I suspect there are many comments that we should update as a result of 
this work.  Essentially there's many throughout GCC of the form "On a 
big-endian target" and the like.  With these changes it's more a 
property of the data -- the vast majority of the time the data's 
property is the same as the target, but with this patch it can vary.

I just happened to spot one in varasm.c::output_constructor_bitfield, as 
the comment started shortly after some code this patch changes.  No 
doubt there's others.  I'm torn whether or not to ask you to scan and 
update them.  It's a lot of work and I'm not terribly sure how valuable 
it'll generally be.

Thanks for not trying too hard to optimize this stuff, it makes the 
expmed.c patches (for example) a lot easier to work through :-)

I didn't even try to verify you've got all the paths covered.  I mostly 
tried to make sure the patch didn't break existing code.  I'm going to 
assume that the patch essentially works and that if there's paths 
missing where reversal is needed that you'll take care of them as 
they're exposed.

I'm a bit dismayed at the number of changes to fold-const.c, but 
presumably there's no good way around them.  I probably would have 
looked for a way to punt earlier (that may not be trivial though). 
Given you've done the work, no need to undo it now.

Throughout the code, in cases where you've just added an argument, I've 
assumed you've passed it correctly in the callers -- I largely glossed 
over those with that assumption in mind.

I think this patch is fine.


Jeff