Re: [hsa merge 09/10] Majority of the HSA back-end

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Jan 13, 2016 at 06:39:34PM +0100, Martin Jambor wrote:
> +struct function_linkage_pair
> +{
> +  function_linkage_pair (tree decl, unsigned int off):

I would have expected a space before :, or better yet on the next line:
  function_linkage_pair (tree decl, unsigned int off)
    : function_decl (decl), offset (off) {}

> +  total_size = sizeof (BrigSectionHeader) + strlen(section_name);

Space between function name and (
(multiple times in the file).
> +void*

Space before *.

> +hsa_brig_section::get_ptr_by_offset (unsigned int offset)
> +{
> +  gcc_assert (offset < total_size);
> +
> +  offset -= header_byte_delta;
> +  unsigned int i;
> +
> +  for (i = 0; offset >= chunks[i].size; i++)
> +    offset -= chunks[i].size;

The vertical spacing looks just weird.  Either declare i first
in the function and put empty line after that, or put empty line
before the declaration and no empty line between the declaration and for,
or declare i inside of for.
> +     r = r * 67 + (unsigned)ds->s[i] - 113;
> +  r = r * 67 + (unsigned)ds->prefix - 113;

Space after ) (twice).

> +  return r;
> +}
> +
> +/* Returns nonzero if DS1 and DS2 are equal.  */
> +
> +inline bool
> +brig_string_slot_hasher::equal (const value_type ds1, const compare_type ds2)
> +{
> +  if (ds1->len == ds2->len)
> +    return ds1->prefix == ds2->prefix && memcmp (ds1->s, ds2->s, ds1->len) == 0;

Too long line.

> +  /* XXX Sanitize the names without all the strdup.  */

Do you plan to work on these XXXs?  Doesn't have to be right away.

> +	  char *modname2;
> +	  asprintf (&modname2, "%s_%s", modname, part);
> +	  free (modname);

Better
	  modname = concat (modname, "_", part, NULL);
?
> +	  modname = modname2;

> +      char buf[64];
> +      sprintf (buf, "__%s_%i", hsa_seg_name (symbol->m_segment),
> +	       symbol->m_name_number);

I'd feel better with a snprintf here.

> +      name_offset = brig_emit_string (buf, prefix);
> +    }
> +
> +  dirvar.name = htole32 (name_offset);
> +  dirvar.init = 0;
> +  dirvar.type = htole16 (symbol->m_type);
> +  dirvar.segment = symbol->m_segment;
> +  /* TODO: Once we are able to access global variables, we must copy their
> +     alignment.  */
> +  dirvar.align = MAX (hsa_natural_alignment (dirvar.type),
> +		      (BrigAlignment8_t) BRIG_ALIGNMENT_4);
> +  dirvar.linkage = symbol->m_linkage;
> +  dirvar.dim.lo = (uint32_t) symbol->m_dim;
> +  dirvar.dim.hi = (uint32_t) ((unsigned long long) symbol->m_dim >> 32);

This makes IMHO too specific host assumptions about long long.
m_dim is UHWI, why not just symbol->m_dim >> 32 without any cast
(I think we rely on unsigned HOST_WIDE_INT being at least 64-bit pretty much
everywhere), or symbol->m_dim >> 16 >> 16?
Or cast to (uint64_t) instead.

> +      /* Internal function.  */
> +      hsa_internal_fn **slot = hsa_emitted_internal_decls->find_slot
> +	(f->m_internal_fn, INSERT);

Again, IMHO if you can easily avoid splitting function decl or ptr and
opening ( to multiple lines, you IMHO should.

      hsa_internal_fn **slot
	= hsa_emitted_internal_decls->find_slot (f->m_internal_fn, INSERT);

> +  char buf[32];
> +
> +  lbldir.base.byteCount = htole16 (sizeof (lbldir));
> +  lbldir.base.kind = htole16 (BRIG_KIND_DIRECTIVE_LABEL);
> +  sprintf (buf, "BB_%u_%i", DECL_UID (current_function_decl), hbb->m_index);

Again, please use snprintf.

> +  unsigned data_len = tree_to_uhwi (TYPE_SIZE (type))/BITS_PER_UNIT;

Spaces around / .

> +  if (INTEGRAL_TYPE_P (type)
> +      || (POINTER_TYPE_P (type) && TREE_CODE (value) == INTEGER_CST))

This looks weird.  You are clearly assuming INTEGER_CST in the code,
but checking it only for POINTER_TYPE_P and not for INTEGRAL_TYPE_P.

> +    switch (data_len)
> +      {
> +      case 1:
> +	bytes.b8 = (uint8_t) TREE_INT_CST_LOW (value);
> +	break;
> +      case 2:
> +	bytes.b16 = (uint16_t) TREE_INT_CST_LOW (value);
> +	break;
> +      case 4:
> +	bytes.b32 = (uint32_t) TREE_INT_CST_LOW (value);
> +	break;
> +      case 8:
> +	bytes.b64 = (uint64_t) int_cst_value (value);

This will ICE on 64-bit unsigned constants, because int_cst_value I think
returns signed HWI, not unsigned.  Why not just TREE_INT_CST_LOW too?

> +	  unsigned actual;
> +	  actual = emit_immediate_scalar_to_buffer
> +	    (VECTOR_CST_ELT (value, i), p, 0);

Better

	  unsigned actual
	    = emit_immediate_scalar_to_buffer (VECTOR_CST_ELT (value, i),
					       p, 0);

> +      unsigned actual;
> +      actual = emit_immediate_scalar_to_buffer
> +	(TREE_REALPART (value), p, total_len / 2);

Likewise.

> +	  unsigned actual = emit_immediate_scalar_to_buffer
> +	    (CONSTRUCTOR_ELT (value, i)->value, p, 0);

Similarly.

> +      out.offset.hi = htole32 (((long long) addr->m_imm_offset) >> 32);

See above.

> +  repr.modifier.allBits = 0 ;

No space before ;

> +  repr.modifier.allBits = 0 ;

Ditto.

> +  else
> +    repr.sourceType = htole16 (as_a <hsa_op_immed *> (cmp->get_op (1))->m_type);

Too long line.

> +  repr.base.operands = htole32
> +    (emit_operands (br->get_op (0), &hsa_bb_for_bb (target)->m_label_ref));

Formatting.

> +  repr.base.operands = htole32
> +    (emit_operands (sbr->get_op (0), sbr->m_label_code_list));

Likewise.

> +  if (call->m_called_function)
> +    function_call_linkage.safe_push
> +      (function_linkage_pair (call->m_called_function,
> +			      call->m_func.m_brig_op_offset));
> +  else
> +    {
> +      hsa_internal_fn *slot = hsa_emitted_internal_decls->find
> +	(call->m_called_internal_fn);

Likewise.  Not sure if you can easily do something with the first one,
perhaps use a temporary?

> +	    insn->m_call_insn->m_result_code_list->m_offsets[0] = htole32
> +	      (emit_directive_variable (insn->m_call_insn->m_output_arg));

Formatting.

> +	  insn->m_operand_list->m_offsets[i - 1] = htole32
> +	    (enqueue_op (insn->get_op (i)));

Ditto.
> +	}
> +
> +      repr.base.operands = htole32 (emit_operands (insn->get_op (0),
> +						   insn->m_operand_list));
> +    }
> +  else if (insn->m_opcode == BRIG_OPCODE_EXPAND)
> +    {
> +      /* Create operand list for packed type.  */
> +      for (unsigned i = 0; i < operand_count - 1; i++)
> +	{
> +	  gcc_checking_assert (insn->get_op (i));
> +	  insn->m_operand_list->m_offsets[i] = htole32
> +	    (enqueue_op (insn->get_op (i)));

Ditto.
> +	}
> +
> +      repr.base.operands = htole32
> +	(emit_operands (insn->m_operand_list,
> +			insn->get_op (insn->operand_count () - 1)));

Ditto.

> +/* Unit constructor and destructor statements.  */
> +
> +static GTY(()) tree hsa_ctor_statements;
> +static GTY(()) tree hsa_dtor_statements;

Perhaps make this cdtor_statements[2] to avoid too many GTY roots?

> +      TREE_TYPE (var_name) = build_array_type
> +	(char_type_node, build_index_type (size_int (len)));

Formatting.

> +      CONSTRUCTOR_APPEND_ELT
> +	(host_functions_vec, NULL_TREE,
> +	 build_fold_addr_expr (hsa_get_host_function (decl)));

Likewise.  Perhaps with a temporary?

> +      TREE_TYPE (kern_name) = build_array_type
> +	(char_type_node, build_index_type (size_int (len)));

Formatting.

> +      kernel_dependencies_vector_type = build_array_type
> +	(build_pointer_type (char_type_node),

Formatting.

> +	      kernel_dependencies_vector_type = build_array_type
> +		(build_pointer_type (char_type_node),
> +		 build_index_type (size_int (count)));

Ditto.

> +		  TREE_TYPE (dependency_name) = build_array_type
> +		    (char_type_node, build_index_type (size_int (len)));
> +
> +		  CONSTRUCTOR_APPEND_ELT
> +		    (kernel_dependencies_vec, NULL_TREE,
> +		     build1 (ADDR_EXPR,
> +			     build_pointer_type (TREE_TYPE (dependency_name)),
> +			     dependency_name));

Ditto several times.

> +  offset_int func_table_size = wi::to_offset (TYPE_SIZE_UNIT (ptr_type_node))
> +    * kernel_count;

  offset_int func_table_size
    = wi::to_offset (TYPE_SIZE_UNIT (ptr_type_node)) * kernel_count;
instead?

> +  tree offload_register = builtin_decl_explicit
> +    (BUILT_IN_GOMP_OFFLOAD_REGISTER);

Formatting.

> +/* Required HSA section alignment. */
> +
> +#define HSA_SECTION_ALIGNMENT 16

Shouldn't this be at the top of the file?

> +      BrigOperandCodeRef *code_ref = (BrigOperandCodeRef *)
> +	(brig_operand.get_ptr_by_offset (p.offset));

      BrigOperandCodeRef *code_ref
	= (BrigOperandCodeRef *) (brig_operand.get_ptr_by_offset (p.offset));
instead?

> +    for (hash_map <tree, BrigDirectiveExecutable *>::iterator it =
> +	 emitted_declarations->begin (); it != emitted_declarations->end ();

= goes on the next line.  And I think the condition should go on yet another
line, either together with ++it, or on its own.
> +	 ++it)

> +  data_padding = HSA_SECTION_ALIGNMENT
> +    - brig_data.total_size % HSA_SECTION_ALIGNMENT;
> +  code_padding = HSA_SECTION_ALIGNMENT
> +    - brig_code.total_size % HSA_SECTION_ALIGNMENT;
> +  operand_padding = HSA_SECTION_ALIGNMENT
> +    - brig_operand.total_size % HSA_SECTION_ALIGNMENT;
> +
> +  uint64_t module_size = sizeof (module_header) + sizeof (section_index)
> +    + brig_data.total_size + data_padding
> +    + brig_code.total_size + code_padding
> +    + brig_operand.total_size + operand_padding;

Put = on the next line in each case, or use
  var = (something
         +- something else ...);

> +  memset (padding, 0, sizeof(padding));

Missing space before (.

> +  bool unsigned_int_type = (BRIG_TYPE_U8 | BRIG_TYPE_U16 | BRIG_TYPE_U32
> +    | BRIG_TYPE_U64) & imm->m_type;

See above.

> +unsigned HOST_WIDE_INT
> +hsa_symbol::total_byte_size ()
> +{
> +  unsigned HOST_WIDE_INT s = hsa_type_bit_size (~BRIG_TYPE_ARRAY_MASK & m_type);

Too long line.
> +  gcc_assert (s % BITS_PER_UNIT == 0);
> +  s /= BITS_PER_UNIT;
> +
> +  if (m_dim)
> +    s *= m_dim;
> +
> +  return s;
> +}
> +
> +/* Forward declaration.  */
> +
> +static BrigType16_t
> +hsa_type_for_tree_type (const_tree type, unsigned HOST_WIDE_INT *dim_p,
> +			bool min32int);
> +
> +void
> +hsa_symbol::fillup_for_decl (tree decl)
> +{
> +  m_decl = decl;
> +  m_type = hsa_type_for_tree_type (TREE_TYPE (decl), &m_dim, false);
> +
> +  if (hsa_seen_error ())
> +    m_seen_error = true;
> +}
> +
> +/* Constructor of class representing global HSA function/kernel information and
> +   state.  FNDECL is function declaration, KERNEL_P is true if the function
> +   is going to become a HSA kernel.  If the function has body, SSA_NAMES_COUNT
> +   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): m_name (NULL),

Space before : or : on the next line.

> +hsa_function_representation::hsa_function_representation (hsa_internal_fn *fn):

Ditto.

> +#define HSA_WARN_MEMORY_ROUTINE "OpenMP device memory library routines have " \
> +  "undefined semantics within target regions, support for HSA ignores them"

Well, if you don't support them in HSA target regions, you'd better punt and
not error on them.

> +#define HSA_WARN_AFFINITY "Support for HSA does not implement OpenMP affinity " \
> +  "featerues"

Typo - features

> +  omp_simple_builtins = new hash_map <nofree_string_hash, omp_simple_builtin>
> +    ();

Formatting.
> +
> +  omp_simple_builtin omp_builtins[] =
> +    {
> +      omp_simple_builtin
> +	("omp_get_initial_device", NULL, false,
> +	 new hsa_op_immed (GOMP_DEVICE_HOST, (BrigType16_t) BRIG_TYPE_S32)),

Ditto (and several times below).

> +  bool is_in_global_vars = TREE_CODE (decl) == VAR_DECL && is_global_var (decl);

Too long line.

> +  hsa_function_summary *s = hsa_summaries->get (cgraph_node::get_create (decl));

Ditto.

> +hsa_op_base::hsa_op_base (BrigKind16_t k): m_next (NULL), m_brig_op_offset (0),
> +  m_kind (k)

Space before : or put : on next line together with all the non-static data
member initializers.

> +/* Constructor of class representing HSA immediate values.  INTEGER_VALUE is the
> +   integer representation of the immediate value.  TYPE is BRIG type.  */
> +
> +hsa_op_immed::hsa_op_immed (HOST_WIDE_INT integer_value, BrigType16_t type)
> +  : hsa_op_with_type (BRIG_KIND_OPERAND_CONSTANT_BYTES, type),
> +  m_tree_value (NULL), m_brig_repr (NULL)

I'd say m_tree_value should be indented by 4 spaces instead of 2, below
hsa_op_with_type.

> +hsa_op_address::hsa_op_address (hsa_symbol *sym, hsa_op_reg *r,
> +				HOST_WIDE_INT offset)
> +  : hsa_op_base (BRIG_KIND_OPERAND_ADDRESS), m_symbol (sym), m_reg (r),
> +  m_imm_offset (offset)

See above (several times below too).

> +hsa_insn_br::hsa_insn_br (hsa_op_reg *ctrl)
> +: hsa_insn_basic (1, BRIG_OPCODE_CBR, BRIG_TYPE_B1, ctrl),
> +  m_width (BRIG_WIDTH_1)

Shift the 2 lines by 2 columns to the right (more times below).

> +      hsa_op_reg *tmp;
> +      tmp = new hsa_op_reg (hsa_get_segment_addr_type
> +			    (addr->m_symbol->m_segment));

      hsa_op_reg *tmp
	= new hsa_op_reg (hsa_get_segment_addr_type (addr->m_symbol->m_segment));
is too long, so perhaps a temporary?

> +  if (flag_tm)
> +    {
> +      HSA_SORRY_AT (UNKNOWN_LOCATION,
> +		    "support for HSA does not implement transactional memory");
> +      goto fail;
> +    }

Wouldn't (at least incrementally) be it much better to just give up if you
see any of the problematic constructs in the function, rather than
just giving up whenever -fgnu-tm is used?

> +      char *b = XNEWVEC (char, 64);
> +      sprintf (b, "__hsa_anonymous_%i", DECL_UID (decl));
> +      const char *ggc_str = ggc_alloc_string (b, strlen (b) + 1);
> +      free (b);

Ugh, why not just char b[64]; instead?  And use snprintf...
And then ggc_strdup?

> +unsigned
> +hsa_internal_fn::get_arity ()
> +{
> +  switch (m_fn)
> +    {
> +    case IFN_ACOS:
> +    case IFN_ASIN:
> +    case IFN_ATAN:
> +    case IFN_COS:
> +    case IFN_EXP:
> +    case IFN_EXP10:
> +    case IFN_EXP2:
> +    case IFN_EXPM1:
> +    case IFN_LOG:
> +    case IFN_LOG10:
> +    case IFN_LOG1P:
> +    case IFN_LOG2:
> +    case IFN_LOGB:
> +    case IFN_SIGNIFICAND:
> +    case IFN_SIN:
> +    case IFN_SQRT:
> +    case IFN_TAN:
> +    case IFN_CEIL:
> +    case IFN_FLOOR:
> +    case IFN_NEARBYINT:
> +    case IFN_RINT:
> +    case IFN_ROUND:
> +    case IFN_TRUNC:
> +      return 1;
> +    case IFN_ATAN2:
> +    case IFN_COPYSIGN:
> +    case IFN_FMOD:
> +    case IFN_POW:
> +    case IFN_REMAINDER:
> +    case IFN_SCALB:
> +    case IFN_LDEXP:
> +      return 2;
> +      break;
> +    case IFN_CLRSB:
> +    case IFN_CLZ:
> +    case IFN_CTZ:
> +    case IFN_FFS:
> +    case IFN_PARITY:
> +    case IFN_POPCOUNT:
> +    default:
> +      gcc_unreachable ();

There are various other IFNs (e.g. for __builtin_{add,sub,mul}_overflow,
lots of others).  How do you ensure you don't ICE on those?

Generally, can I ask you to search for each of the categories of formatting
and other issues in other spots and attempt to clean up or fix them too?
Like for the call formatting generally search for ERE '^[[:blank:]]*\(',
for long lines just use contrib/check_GNU_style.sh, look for
'=[[:blank:]]*$', '^:', '\):', look around even '^  :', etc.?

	Jakub