Message ID | 56E7F907.9090309@suse.cz |
---|---|
State | New |
Headers | show |
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 <mliska@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
On 03/17/2016 07:21 PM, Martin Jambor wrote: > 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); > } Hi Martin. I like the refactoring you did! > > 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. Yes, it can happen in a situation where a pair of functions utilizes a global variable and HSA emission fails for the first one. Installed as r234362. Martin > > Thanks a lot, > > Martin
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); + return *slot; } else diff --git a/gcc/hsa.h b/gcc/hsa.h index 6a7c651..1d6baab 100644 --- a/gcc/hsa.h +++ b/gcc/hsa.h @@ -110,6 +110,9 @@ struct hsa_symbol /* Symbol allocation. */ BrigAllocation m_allocation; + /* Flag used for global variables if a variable is already emitted or not. */ + bool m_emitted_to_brig; + private: /* Default constructor. */ hsa_symbol ();