Re: [PATCH 2/2] HSA: handle alignment of string builtins (PR hsa/70391)

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Wed, Mar 23, 2016 at 02:43:17PM +0100, Martin Liska wrote:
> gcc/ChangeLog:
> 
> 2016-03-23  Martin Liska  <mliska@suse.cz>
> 
> 	PR hsa/70391
> 	* hsa-gen.c (hsa_function_representation::update_cfg): New
> 	function.
> 	(convert_addr_to_flat_segment): Likewise.
> 	(gen_hsa_memory_set): New alignment argument.
> 	(gen_hsa_ctor_assignment): Likewise.
> 	(gen_hsa_insns_for_single_assignment): Provide alignment
> 	to gen_hsa_ctor_assignment.
> 	(gen_hsa_insns_for_direct_call): Add new argument.
> 	(expand_lhs_of_string_op): New function.
> 	(expand_string_operation_builtin): Likewise.
> 	(expand_memory_copy): New function.
> 	(expand_memory_set): New function.
> 	(gen_hsa_insns_for_call): Use HOST_WIDE_INT.
> 	(convert_switch_statements): Change signature.
> 	(generate_hsa): Use a return value of the function.
> 	(pass_gen_hsail::execute): Do not call
> 	convert_switch_statements here.
> 	* hsa-regalloc.c (hsa_regalloc): Call update_cfg.
> 	* hsa.h (hsa_function_representation::m_need_cfg_update):
> 	New flag.
> 	(hsa_function_representation::update_cfg): New function.

As we already discussed, update_cfg and m_need_cfg_update should
really be called differently, because CFG has already been modified
and only dominance needs to be re-computed.  If you havent't thought
about any names yet, what about m_modified_cfg and update_dominance() ?


> ---
>  gcc/hsa-gen.c      | 372 ++++++++++++++++++++++++++++++++++++++---------------
>  gcc/hsa-regalloc.c |   1 +
>  gcc/hsa.h          |   9 +-
>  3 files changed, 275 insertions(+), 107 deletions(-)
> 
> diff --git a/gcc/hsa-gen.c b/gcc/hsa-gen.c
> index db39813..db7fc3d 100644
> --- a/gcc/hsa-gen.c
> +++ b/gcc/hsa-gen.c
> @@ -214,7 +214,7 @@ hsa_symbol::fillup_for_decl (tree decl)
>     should be set to number of SSA names used in the function.  */
>  
>  hsa_function_representation::hsa_function_representation
> -  (tree fdecl, bool kernel_p, unsigned ssa_names_count)
> +  (tree fdecl, bool kernel_p, unsigned ssa_names_count, bool need_cfg_update)
>    : m_name (NULL),
>      m_reg_count (0), m_input_args (vNULL),
>      m_output_arg (NULL), m_spill_symbols (vNULL), m_global_symbols (vNULL),
> @@ -223,7 +223,8 @@ hsa_function_representation::hsa_function_representation
>      m_in_ssa (true), m_kern_p (kernel_p), m_declaration_p (false),
>      m_decl (fdecl), m_internal_fn (NULL), m_shadow_reg (NULL),
>      m_kernel_dispatch_count (0), m_maximum_omp_data_size (0),
> -    m_seen_error (false), m_temp_symbol_count (0), m_ssa_map ()
> +    m_seen_error (false), m_temp_symbol_count (0), m_ssa_map (),
> +    m_need_cfg_update (need_cfg_update)
>  {
>    int sym_init_len = (vec_safe_length (cfun->local_decls) / 2) + 1;;
>    m_local_symbols = new hash_table <hsa_noop_symbol_hasher> (sym_init_len);
> @@ -319,6 +320,16 @@ hsa_function_representation::init_extra_bbs ()
>    hsa_init_new_bb (EXIT_BLOCK_PTR_FOR_FN (cfun));
>  }
>  
> +void
> +hsa_function_representation::update_cfg ()
> +{
> +  if (m_need_cfg_update)
> +    {
> +      free_dominance_info (CDI_DOMINATORS);
> +      calculate_dominance_info (CDI_DOMINATORS);
> +    }
> +}
> +
>  hsa_symbol *
>  hsa_function_representation::create_hsa_temporary (BrigType16_t type)
>  {
> @@ -2246,30 +2257,14 @@ gen_hsa_addr_for_arg (tree tree_type, int index)
>    return new hsa_op_address (sym);
>  }
>  
> -/* Generate HSA instructions that calculate address of VAL including all
> -   necessary conversions to flat addressing and place the result into DEST.
> +/* Generate HSA instructions that process all necessary conversions
> +   of an ADDR to flat addressing and place the result into DEST.
>     Instructions are appended to HBB.  */
>  
>  static void
> -gen_hsa_addr_insns (tree val, hsa_op_reg *dest, hsa_bb *hbb)
> +convert_addr_to_flat_segment (hsa_op_address *addr, hsa_op_reg *dest,
> +			      hsa_bb *hbb)
>  {
> -  /* Handle cases like tmp = NULL, where we just emit a move instruction
> -     to a register.  */
> -  if (TREE_CODE (val) == INTEGER_CST)
> -    {
> -      hsa_op_immed *c = new hsa_op_immed (val);
> -      hsa_insn_basic *insn = new hsa_insn_basic (2, BRIG_OPCODE_MOV,
> -						 dest->m_type, dest, c);
> -      hbb->append_insn (insn);
> -      return;
> -    }
> -
> -  hsa_op_address *addr;
> -
> -  gcc_assert (dest->m_type == hsa_get_segment_addr_type (BRIG_SEGMENT_FLAT));
> -  if (TREE_CODE (val) == ADDR_EXPR)
> -    val = TREE_OPERAND (val, 0);
> -  addr = gen_hsa_addr (val, hbb);
>    hsa_insn_basic *insn = new hsa_insn_basic (2, BRIG_OPCODE_LDA);
>    insn->set_op (1, addr);
>    if (addr->m_symbol && addr->m_symbol->m_segment != BRIG_SEGMENT_GLOBAL)
> @@ -2298,6 +2293,34 @@ gen_hsa_addr_insns (tree val, hsa_op_reg *dest, hsa_bb *hbb)
>      }
>  }
>  
> +/* Generate HSA instructions that calculate address of VAL including all
> +   necessary conversions to flat addressing and place the result into DEST.
> +   Instructions are appended to HBB.  */
> +
> +static void
> +gen_hsa_addr_insns (tree val, hsa_op_reg *dest, hsa_bb *hbb)
> +{
> +  /* Handle cases like tmp = NULL, where we just emit a move instruction
> +     to a register.  */
> +  if (TREE_CODE (val) == INTEGER_CST)
> +    {
> +      hsa_op_immed *c = new hsa_op_immed (val);
> +      hsa_insn_basic *insn = new hsa_insn_basic (2, BRIG_OPCODE_MOV,
> +						 dest->m_type, dest, c);
> +      hbb->append_insn (insn);
> +      return;
> +    }
> +
> +  hsa_op_address *addr;
> +
> +  gcc_assert (dest->m_type == hsa_get_segment_addr_type (BRIG_SEGMENT_FLAT));
> +  if (TREE_CODE (val) == ADDR_EXPR)
> +    val = TREE_OPERAND (val, 0);
> +  addr = gen_hsa_addr (val, hbb);
> +
> +  convert_addr_to_flat_segment (addr, dest, hbb);
> +}
> +
>  /* Return an HSA register or HSA immediate value operand corresponding to
>     gimple operand OP.  */
>  
> @@ -2728,9 +2751,9 @@ gen_hsa_insns_for_store (tree lhs, hsa_op_base *src, hsa_bb *hbb)
>  }
>  
>  /* Generate memory copy instructions that are going to be used
> -   for copying a HSA symbol SRC_SYMBOL (or SRC_REG) to TARGET memory,
> +   for copying a SRC memory to TARGET memory,
>     represented by pointer in a register.  MIN_ALIGN is minimal alignment
> -   of provided HSA addresses. */
> +   of provided HSA addresses.  */
>  
>  static void
>  gen_hsa_memory_copy (hsa_bb *hbb, hsa_op_address *target, hsa_op_address *src,
> @@ -2792,17 +2815,19 @@ build_memset_value (unsigned HOST_WIDE_INT constant, unsigned byte_size)
>  }
>  
>  /* Generate memory set instructions that are going to be used
> -   for setting a CONSTANT byte value to TARGET memory of SIZE bytes.  */
> +   for setting a CONSTANT byte value to TARGET memory of SIZE bytes.
> +   MIN_ALIGN is minimal alignment of provided HSA addresses.  */
>  
>  static void
>  gen_hsa_memory_set (hsa_bb *hbb, hsa_op_address *target,
>  		    unsigned HOST_WIDE_INT constant,
> -		    unsigned size)
> +		    unsigned size, BrigAlignment8_t min_align)
>  {
>    hsa_op_address *addr;
>    hsa_insn_mem *mem;
>  
>    unsigned offset = 0;
> +  unsigned min_byte_align = hsa_byte_alignment (min_align);
>  
>    while (size)
>      {
> @@ -2816,6 +2841,9 @@ gen_hsa_memory_set (hsa_bb *hbb, hsa_op_address *target,
>        else
>  	s = 1;
>  
> +      if (s > min_byte_align)
> +	s = min_byte_align;
> +
>        addr = new hsa_op_address (target->m_symbol, target->m_reg,
>  				 target->m_imm_offset + offset);
>  
> @@ -2832,10 +2860,12 @@ gen_hsa_memory_set (hsa_bb *hbb, hsa_op_address *target,
>  
>  /* Generate HSAIL instructions for a single assignment
>     of an empty constructor to an ADDR_LHS.  Constructor is passed as a
> -   tree RHS and all instructions are appended to HBB.  */
> +   tree RHS and all instructions are appended to HBB.  ALIGN is
> +   alignment of the address.  */
>  
>  void
> -gen_hsa_ctor_assignment (hsa_op_address *addr_lhs, tree rhs, hsa_bb *hbb)
> +gen_hsa_ctor_assignment (hsa_op_address *addr_lhs, tree rhs, hsa_bb *hbb,
> +			 BrigAlignment8_t align)
>  {
>    if (vec_safe_length (CONSTRUCTOR_ELTS (rhs)))
>      {
> @@ -2845,7 +2875,7 @@ gen_hsa_ctor_assignment (hsa_op_address *addr_lhs, tree rhs, hsa_bb *hbb)
>      }
>  
>    unsigned size = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (rhs)));
> -  gen_hsa_memory_set (hbb, addr_lhs, 0, size);
> +  gen_hsa_memory_set (hbb, addr_lhs, 0, size, align);
>  }
>  
>  /* Generate HSA instructions for a single assignment of RHS to LHS.
> @@ -2879,7 +2909,7 @@ gen_hsa_insns_for_single_assignment (tree lhs, tree rhs, hsa_bb *hbb)
>  							  &lhs_align);
>  
>        if (TREE_CODE (rhs) == CONSTRUCTOR)
> -	gen_hsa_ctor_assignment (addr_lhs, rhs, hbb);
> +	gen_hsa_ctor_assignment (addr_lhs, rhs, hbb, lhs_align);
>        else
>  	{
>  	  BrigAlignment8_t rhs_align;
> @@ -3523,10 +3553,13 @@ get_format_argument_type (tree formal_arg_type, BrigType16_t actual_arg_type)
>  
>  /* Generate HSA instructions for a direct call instruction.
>     Instructions will be appended to HBB, which also needs to be the
> -   corresponding structure to the basic_block of STMT.  */
> +   corresponding structure to the basic_block of STMT.
> +   If ASSIGN_LHS is set to true, assignment to a LHS of the called function
> +   is not processed.  */

I'm not sure the last sentence is really helpful becasue the word
"processed" is not very informative and also because I think it means
the opposite of what the code actually does (it depends on what being
processed means, I suppose).  Can you perhaps replace it with "If
ASSIGN_LHS is false, do not copy HSA function result argument into the
corresponding HSA representation of the gimple statement LHS."

Otherwise, the patch is fine, thanks.

Martin