On 03/24/2016 03:59 PM, Martin Liška wrote: > Hello. > > Current HSA back-end wrongly handles memory stores. Although, we properly > identify > that an immediate operand needs to respect type of a memory store instruction > it belongs to, > the binary representation of the operand is not updated. > > Following patch delays emission of the binary representation and updates > hsa_op_immmed::m_brig_repr_size > every time the m_type field of the operand is updated. > > I've been testing the patch, ready after it finishes? > > Thanks, > Martin
Hi. I've prepared v2 of the patch, where I postpone emission of binary representation of an immediate value. The patch is pre-approved by Martin Jambor. Installed as r234647. Thanks, Martin
>From 1b77faf3ae850b2620b88fb656c372e7736540bd Mon Sep 17 00:00:00 2001 From: marxin <mli...@suse.cz> Date: Thu, 24 Mar 2016 15:41:59 +0100 Subject: [PATCH] Fix PR hsa/70399 gcc/ChangeLog: 2016-03-24 Martin Liska <mli...@suse.cz> PR hsa/70399 * hsa-brig.c (hsa_op_immed::emit_to_buffer): Emit either a tree value or an immediate integer value to a buffer that is eventually copied to a BRIG section. (emit_immediate_operand): Call the function here. * hsa-dump.c (dump_hsa_immed): Remove checking assert. * hsa-gen.c (hsa_op_immed::hsa_op_immed): Remove initialization of class' fields that are removed. (hsa_op_immed::~hsa_op_immed): Remove deinitialization. * hsa.h (class hsa_op_immed): Remove m_brig_repr and m_brig_repr_size fields. --- gcc/hsa-brig.c | 133 ++++++++++++++++++++++++++++++++++++++------------------- gcc/hsa-dump.c | 2 - gcc/hsa-gen.c | 68 +++++++---------------------- gcc/hsa.h | 12 +++--- 4 files changed, 109 insertions(+), 106 deletions(-) diff --git a/gcc/hsa-brig.c b/gcc/hsa-brig.c index 9b6c0b8..71d4f66 100644 --- a/gcc/hsa-brig.c +++ b/gcc/hsa-brig.c @@ -932,62 +932,101 @@ emit_immediate_scalar_to_buffer (tree value, char *data, unsigned need_len) return len; } -void -hsa_op_immed::emit_to_buffer (tree value) +char * +hsa_op_immed::emit_to_buffer (unsigned *brig_repr_size) { - unsigned total_len = m_brig_repr_size; - - /* As we can have a constructor with fewer elements, fill the memory - with zeros. */ - m_brig_repr = XCNEWVEC (char, total_len); - char *p = m_brig_repr; + char *brig_repr; + *brig_repr_size = hsa_get_imm_brig_type_len (m_type); - if (TREE_CODE (value) == VECTOR_CST) + if (m_tree_value != NULL_TREE) { - int i, num = VECTOR_CST_NELTS (value); - for (i = 0; i < num; i++) + /* Update brig_repr_size for special tree values. */ + if (TREE_CODE (m_tree_value) == STRING_CST) + *brig_repr_size = TREE_STRING_LENGTH (m_tree_value); + else if (TREE_CODE (m_tree_value) == CONSTRUCTOR) + *brig_repr_size + = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (m_tree_value))); + + unsigned total_len = *brig_repr_size; + + /* As we can have a constructor with fewer elements, fill the memory + with zeros. */ + brig_repr = XCNEWVEC (char, total_len); + char *p = brig_repr; + + if (TREE_CODE (m_tree_value) == VECTOR_CST) { + int i, num = VECTOR_CST_NELTS (m_tree_value); + for (i = 0; i < num; i++) + { + tree v = VECTOR_CST_ELT (m_tree_value, i); + unsigned actual = emit_immediate_scalar_to_buffer (v, p, 0); + total_len -= actual; + p += actual; + } + /* Vectors should have the exact size. */ + gcc_assert (total_len == 0); + } + else if (TREE_CODE (m_tree_value) == STRING_CST) + memcpy (brig_repr, TREE_STRING_POINTER (m_tree_value), + TREE_STRING_LENGTH (m_tree_value)); + else if (TREE_CODE (m_tree_value) == COMPLEX_CST) + { + gcc_assert (total_len % 2 == 0); unsigned actual; actual - = emit_immediate_scalar_to_buffer (VECTOR_CST_ELT (value, i), p, 0); - total_len -= actual; + = emit_immediate_scalar_to_buffer (TREE_REALPART (m_tree_value), p, + total_len / 2); + + gcc_assert (actual == total_len / 2); p += actual; + + actual + = emit_immediate_scalar_to_buffer (TREE_IMAGPART (m_tree_value), p, + total_len / 2); + gcc_assert (actual == total_len / 2); } - /* Vectors should have the exact size. */ - gcc_assert (total_len == 0); - } - else if (TREE_CODE (value) == STRING_CST) - memcpy (m_brig_repr, TREE_STRING_POINTER (value), - TREE_STRING_LENGTH (value)); - else if (TREE_CODE (value) == COMPLEX_CST) - { - gcc_assert (total_len % 2 == 0); - unsigned actual; - actual - = emit_immediate_scalar_to_buffer (TREE_REALPART (value), p, - total_len / 2); - - gcc_assert (actual == total_len / 2); - p += actual; - - actual - = emit_immediate_scalar_to_buffer (TREE_IMAGPART (value), p, - total_len / 2); - gcc_assert (actual == total_len / 2); + else if (TREE_CODE (m_tree_value) == CONSTRUCTOR) + { + unsigned len = vec_safe_length (CONSTRUCTOR_ELTS (m_tree_value)); + for (unsigned i = 0; i < len; i++) + { + tree v = CONSTRUCTOR_ELT (m_tree_value, i)->value; + unsigned actual = emit_immediate_scalar_to_buffer (v, p, 0); + total_len -= actual; + p += actual; + } + } + else + emit_immediate_scalar_to_buffer (m_tree_value, p, total_len); } - else if (TREE_CODE (value) == CONSTRUCTOR) + else { - unsigned len = vec_safe_length (CONSTRUCTOR_ELTS (value)); - for (unsigned i = 0; i < len; i++) + hsa_bytes bytes; + + switch (*brig_repr_size) { - tree v = CONSTRUCTOR_ELT (value, i)->value; - unsigned actual = emit_immediate_scalar_to_buffer (v, p, 0); - total_len -= actual; - p += actual; + case 1: + bytes.b8 = (uint8_t) m_int_value; + break; + case 2: + bytes.b16 = (uint16_t) m_int_value; + break; + case 4: + bytes.b32 = (uint32_t) m_int_value; + break; + case 8: + bytes.b64 = (uint64_t) m_int_value; + break; + default: + gcc_unreachable (); } + + brig_repr = XNEWVEC (char, *brig_repr_size); + memcpy (brig_repr, &bytes, *brig_repr_size); } - else - emit_immediate_scalar_to_buffer (value, p, total_len); + + return brig_repr; } /* Emit an immediate BRIG operand IMM. The BRIG type of the immediate might @@ -999,17 +1038,21 @@ hsa_op_immed::emit_to_buffer (tree value) static void emit_immediate_operand (hsa_op_immed *imm) { + unsigned brig_repr_size; + char *brig_repr = imm->emit_to_buffer (&brig_repr_size); struct BrigOperandConstantBytes out; memset (&out, 0, sizeof (out)); out.base.byteCount = lendian16 (sizeof (out)); out.base.kind = lendian16 (BRIG_KIND_OPERAND_CONSTANT_BYTES); - uint32_t byteCount = lendian32 (imm->m_brig_repr_size); + uint32_t byteCount = lendian32 (brig_repr_size); out.type = lendian16 (imm->m_type); out.bytes = lendian32 (brig_data.add (&byteCount, sizeof (byteCount))); brig_operand.add (&out, sizeof (out)); - brig_data.add (imm->m_brig_repr, imm->m_brig_repr_size); + brig_data.add (brig_repr, brig_repr_size); brig_data.round_size_up (4); + + free (brig_repr); } /* Emit a register BRIG operand REG. */ diff --git a/gcc/hsa-dump.c b/gcc/hsa-dump.c index b69b34d..c8c2523 100644 --- a/gcc/hsa-dump.c +++ b/gcc/hsa-dump.c @@ -657,8 +657,6 @@ dump_hsa_immed (FILE *f, hsa_op_immed *imm) print_generic_expr (f, imm->m_tree_value, 0); else { - gcc_checking_assert (imm->m_brig_repr_size <= 8); - if (unsigned_int_type) fprintf (f, HOST_WIDE_INT_PRINT_DEC, imm->m_int_value); else diff --git a/gcc/hsa-gen.c b/gcc/hsa-gen.c index 72eecf9..4a7d8fb 100644 --- a/gcc/hsa-gen.c +++ b/gcc/hsa-gen.c @@ -1054,8 +1054,7 @@ hsa_op_with_type::get_in_type (BrigType16_t dtype, hsa_bb *hbb) hsa_op_immed::hsa_op_immed (tree tree_val, bool min32int) : hsa_op_with_type (BRIG_KIND_OPERAND_CONSTANT_BYTES, hsa_type_for_tree_type (TREE_TYPE (tree_val), NULL, - min32int)), - m_brig_repr (NULL) + min32int)) { if (hsa_seen_error ()) return; @@ -1065,30 +1064,20 @@ hsa_op_immed::hsa_op_immed (tree tree_val, bool min32int) || TREE_CODE (tree_val) == INTEGER_CST)) || TREE_CODE (tree_val) == CONSTRUCTOR); m_tree_value = tree_val; - m_brig_repr_size = hsa_get_imm_brig_type_len (m_type); - if (TREE_CODE (m_tree_value) == STRING_CST) - m_brig_repr_size = TREE_STRING_LENGTH (m_tree_value); - else if (TREE_CODE (m_tree_value) == CONSTRUCTOR) - { - m_brig_repr_size - = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (m_tree_value))); - - /* Verify that all elements of a constructor are constants. */ - for (unsigned i = 0; - i < vec_safe_length (CONSTRUCTOR_ELTS (m_tree_value)); i++) - { - tree v = CONSTRUCTOR_ELT (m_tree_value, i)->value; - if (!CONSTANT_CLASS_P (v)) - { - HSA_SORRY_AT (EXPR_LOCATION (tree_val), - "HSA ctor should have only constants"); - return; - } - } - } - - emit_to_buffer (m_tree_value); + /* Verify that all elements of a constructor are constants. */ + if (TREE_CODE (m_tree_value) == CONSTRUCTOR) + for (unsigned i = 0; + i < vec_safe_length (CONSTRUCTOR_ELTS (m_tree_value)); i++) + { + tree v = CONSTRUCTOR_ELT (m_tree_value, i)->value; + if (!CONSTANT_CLASS_P (v)) + { + HSA_SORRY_AT (EXPR_LOCATION (tree_val), + "HSA ctor should have only constants"); + return; + } + } } /* Constructor of class representing HSA immediate values. INTEGER_VALUE is the @@ -1096,38 +1085,14 @@ hsa_op_immed::hsa_op_immed (tree tree_val, bool min32int) 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) + m_tree_value (NULL) { gcc_assert (hsa_type_integer_p (type)); m_int_value = integer_value; - m_brig_repr_size = hsa_type_bit_size (type) / BITS_PER_UNIT; - - hsa_bytes bytes; - - switch (m_brig_repr_size) - { - case 1: - bytes.b8 = (uint8_t) m_int_value; - break; - case 2: - bytes.b16 = (uint16_t) m_int_value; - break; - case 4: - bytes.b32 = (uint32_t) m_int_value; - break; - case 8: - bytes.b64 = (uint64_t) m_int_value; - break; - default: - gcc_unreachable (); - } - - m_brig_repr = XNEWVEC (char, m_brig_repr_size); - memcpy (m_brig_repr, &bytes, m_brig_repr_size); } hsa_op_immed::hsa_op_immed () - : hsa_op_with_type (BRIG_KIND_NONE, BRIG_TYPE_NONE), m_brig_repr (NULL) + : hsa_op_with_type (BRIG_KIND_NONE, BRIG_TYPE_NONE) { } @@ -1143,7 +1108,6 @@ hsa_op_immed::operator new (size_t) hsa_op_immed::~hsa_op_immed () { - free (m_brig_repr); } /* Change type of the immediate value to T. */ diff --git a/gcc/hsa.h b/gcc/hsa.h index 1d6baab..59bec04 100644 --- a/gcc/hsa.h +++ b/gcc/hsa.h @@ -171,25 +171,23 @@ public: ~hsa_op_immed (); void set_type (BrigKind16_t t); + /* Function returns pointer to a buffer that contains binary representation + of the immeadiate value. The buffer has length of BRIG_SIZE and + a caller is responsible for deallocation of the buffer. */ + char *emit_to_buffer (unsigned *brig_size); + /* Value as represented by middle end. */ tree m_tree_value; /* Integer value representation. */ HOST_WIDE_INT m_int_value; - /* Brig data representation. */ - char *m_brig_repr; - - /* Brig data representation size in bytes. */ - unsigned m_brig_repr_size; - private: /* Make the default constructor inaccessible. */ hsa_op_immed (); /* All objects are deallocated by destroying their pool, so make delete inaccessible too. */ void operator delete (void *) {} - void emit_to_buffer (tree value); }; /* Report whether or not P is a an immediate operand. */ -- 2.7.1