Hi,
On Tue, Mar 15, 2016 at 12:59:03PM +0100, Martin Liska wrote:
> Hi.
>
> As emission of a HSAIL function can fail for various reason (-Whsa),
> we must guarantee that a global variable is declared and at maximum once.
>
> Following patch does that, patch can survive make check-target-libgomp and
> HSAILAsm is happy with BRIG output of declare_target-5.c source file.
>
> Currently, I'm running bootstrap on x86_64-linux-gnu.
> Ready to install after if finishes?
>
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> 2016-03-15 Martin Liska <mli...@suse.cz>
>
> PR hsa/70234
> * hsa-brig.c (emit_function_directives): Mark unemitted
> global variables for emission.
> * hsa-gen.c (hsa_symbol::hsa_symbol): Initialize a new flag.
> (get_symbol_for_decl): Likewise.
> * hsa.h (struct hsa_symbol): New flag.
> ---
> gcc/hsa-brig.c | 2 ++
> gcc/hsa-gen.c | 22 +++++++++++++++++++---
> gcc/hsa.h | 3 +++
> 3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/hsa-brig.c b/gcc/hsa-brig.c
> index 2a301be..9b6c0b8 100644
> --- a/gcc/hsa-brig.c
> +++ b/gcc/hsa-brig.c
> @@ -643,6 +643,8 @@ emit_function_directives (hsa_function_representation *f,
> bool is_declaration)
> if (!f->m_declaration_p)
> for (int i = 0; f->m_global_symbols.iterate (i, &sym); i++)
> {
> + gcc_assert (!sym->m_emitted_to_brig);
> + sym->m_emitted_to_brig = true;
> emit_directive_variable (sym);
> brig_insn_count++;
> }
> diff --git a/gcc/hsa-gen.c b/gcc/hsa-gen.c
> index 5939a57..473d4bd 100644
> --- a/gcc/hsa-gen.c
> +++ b/gcc/hsa-gen.c
> @@ -162,7 +162,7 @@ hsa_symbol::hsa_symbol ()
> m_directive_offset (0), m_type (BRIG_TYPE_NONE),
> m_segment (BRIG_SEGMENT_NONE), m_linkage (BRIG_LINKAGE_NONE), m_dim (0),
> m_cst_value (NULL), m_global_scope_p (false), m_seen_error (false),
> - m_allocation (BRIG_ALLOCATION_AUTOMATIC)
> + m_allocation (BRIG_ALLOCATION_AUTOMATIC), m_emitted_to_brig (false)
> {
> }
>
> @@ -174,7 +174,7 @@ hsa_symbol::hsa_symbol (BrigType16_t type, BrigSegment8_t
> segment,
> m_directive_offset (0), m_type (type), m_segment (segment),
> m_linkage (linkage), m_dim (0), m_cst_value (NULL),
> m_global_scope_p (global_scope_p), m_seen_error (false),
> - m_allocation (allocation)
> + m_allocation (allocation), m_emitted_to_brig (false)
> {
> }
>
> @@ -880,11 +880,27 @@ get_symbol_for_decl (tree decl)
> gcc_checking_assert (slot);
> if (*slot)
> {
> + hsa_symbol *sym = (*slot);
> +
> /* If the symbol is problematic, mark current function also as
> problematic. */
> - if ((*slot)->m_seen_error)
> + if (sym->m_seen_error)
> hsa_fail_cfun ();
>
> + /* PR hsa/70234: If a global variable was marked to be emitted,
> + but HSAIL generation of a function using the variable fails,
> + we should retry to emit the variable in context of a different
> + function.
> +
> + Iterate elements whether a symbol is already in m_global_symbols
> + of not. */
> + for (unsigned i = 0; i < hsa_cfun->m_global_symbols.length (); i++)
> + if (hsa_cfun->m_global_symbols[i] == sym)
> + return *slot;
> +
> + if (is_in_global_vars && !sym->m_emitted_to_brig)
> + hsa_cfun->m_global_symbols.safe_push (sym);
> +
Hopefully the linear search in m_global_symbols never becomes
prohibitively expensive. But it is only necessary when
is_in_global_vars is true, so at least we could do something like:
if (is_in_global_vars && !sym->m_emitted_to_brig)
{
for (unsigned i = 0; i < hsa_cfun->m_global_symbols.length (); i++)
if (hsa_cfun->m_global_symbols[i] == sym)
return *slot;
hsa_cfun->m_global_symbols.safe_push (sym);
}
OK with that change. And even though I have seen the bug only on the
hsa branch, commit the fix to trunk too, I think it can happen there
as well.
Thanks a lot,
Martin
