| Message ID | alpine.LNX.2.20.1606231729280.9974@monopod.intra.ispras.ru |
|---|---|
| State | New |
| Headers | show |
On Thu, 23 Jun 2016, Alexander Monakov wrote: > Hi, > > I've discovered that this assert in my patch was too restrictive: > > + if (DECL_HAS_VALUE_EXPR_P (pv->decl)) > + { > + gcc_checking_assert (lookup_attribute ("omp declare target link", > + DECL_ATTRIBUTES (pv->decl))); > > Testing for the nvptx target uncovered that there's another case where a > global variable would have a value expr: emutls. Sorry for not spotting it > earlier (but at least the new assert did its job). I think we should always > skip here over decls that have value-exprs, just like hard-reg vars are > skipped. The following patch does that. Is this still OK? Ping. > (bootstrapped/regtested on x86-64) > > Alexander > > * cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF. > (output_in_order): Loop over undefined variables too. Output them > via assemble_undefined_decl. Skip variables that correspond to hard > registers or have value-exprs. > * varpool.c (symbol_table::output_variables): Handle undefined > variables together with defined ones. > > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > index 4bfcad7..e30fe6e 100644 > --- a/gcc/cgraphunit.c > +++ b/gcc/cgraphunit.c > @@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind > ORDER_UNDEFINED = 0, > ORDER_FUNCTION, > ORDER_VAR, > + ORDER_VAR_UNDEF, > ORDER_ASM > }; > > @@ -2187,16 +2188,20 @@ output_in_order (bool no_reorder) > } > } > > - FOR_EACH_DEFINED_VARIABLE (pv) > - if (!DECL_EXTERNAL (pv->decl)) > - { > - if (no_reorder && !pv->no_reorder) > - continue; > - i = pv->order; > - gcc_assert (nodes[i].kind == ORDER_UNDEFINED); > - nodes[i].kind = ORDER_VAR; > - nodes[i].u.v = pv; > - } > + /* There is a similar loop in symbol_table::output_variables. > + Please keep them in sync. */ > + FOR_EACH_VARIABLE (pv) > + { > + if (no_reorder && !pv->no_reorder) > + continue; > + if (DECL_HARD_REGISTER (pv->decl) > + || DECL_HAS_VALUE_EXPR_P (pv->decl)) > + continue; > + i = pv->order; > + gcc_assert (nodes[i].kind == ORDER_UNDEFINED); > + nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF; > + nodes[i].u.v = pv; > + } > > for (pa = symtab->first_asm_symbol (); pa; pa = pa->next) > { > @@ -2222,16 +2227,13 @@ output_in_order (bool no_reorder) > break; > > case ORDER_VAR: > -#ifdef ACCEL_COMPILER > - /* Do not assemble "omp declare target link" vars. */ > - if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl) > - && lookup_attribute ("omp declare target link", > - DECL_ATTRIBUTES (nodes[i].u.v->decl))) > - break; > -#endif > nodes[i].u.v->assemble_decl (); > break; > > + case ORDER_VAR_UNDEF: > + assemble_undefined_decl (nodes[i].u.v->decl); > + break; > + > case ORDER_ASM: > assemble_asm (nodes[i].u.a->asm_str); > break; > diff --git a/gcc/varpool.c b/gcc/varpool.c > index ab615fa..e5f991e 100644 > --- a/gcc/varpool.c > +++ b/gcc/varpool.c > @@ -733,11 +733,6 @@ symbol_table::output_variables (void) > > timevar_push (TV_VAROUT); > > - FOR_EACH_VARIABLE (node) > - if (!node->definition > - && !DECL_HAS_VALUE_EXPR_P (node->decl) > - && !DECL_HARD_REGISTER (node->decl)) > - assemble_undefined_decl (node->decl); > FOR_EACH_DEFINED_VARIABLE (node) > { > /* Handled in output_in_order. */ > @@ -747,20 +742,19 @@ symbol_table::output_variables (void) > node->finalize_named_section_flags (); > } > > - FOR_EACH_DEFINED_VARIABLE (node) > + /* There is a similar loop in output_in_order. Please keep them in sync. */ > + FOR_EACH_VARIABLE (node) > { > /* Handled in output_in_order. */ > if (node->no_reorder) > continue; > -#ifdef ACCEL_COMPILER > - /* Do not assemble "omp declare target link" vars. */ > - if (DECL_HAS_VALUE_EXPR_P (node->decl) > - && lookup_attribute ("omp declare target link", > - DECL_ATTRIBUTES (node->decl))) > + if (DECL_HARD_REGISTER (node->decl) > + || DECL_HAS_VALUE_EXPR_P (node->decl)) > continue; > -#endif > - if (node->assemble_decl ()) > - changed = true; > + if (node->definition) > + changed |= node->assemble_decl (); > + else > + assemble_undefined_decl (node->decl); > } > timevar_pop (TV_VAROUT); > return changed; > >
On Fri, 1 Jul 2016, Alexander Monakov wrote: > On Thu, 23 Jun 2016, Alexander Monakov wrote: > > Hi, > > > > I've discovered that this assert in my patch was too restrictive: > > > > + if (DECL_HAS_VALUE_EXPR_P (pv->decl)) > > + { > > + gcc_checking_assert (lookup_attribute ("omp declare target link", > > + DECL_ATTRIBUTES (pv->decl))); > > > > Testing for the nvptx target uncovered that there's another case where a > > global variable would have a value expr: emutls. Sorry for not spotting it > > earlier (but at least the new assert did its job). I think we should always > > skip here over decls that have value-exprs, just like hard-reg vars are > > skipped. The following patch does that. Is this still OK? > > Ping. Ping^2. > > (bootstrapped/regtested on x86-64) > > > > Alexander > > > > * cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF. > > (output_in_order): Loop over undefined variables too. Output them > > via assemble_undefined_decl. Skip variables that correspond to hard > > registers or have value-exprs. > > * varpool.c (symbol_table::output_variables): Handle undefined > > variables together with defined ones. > > > > diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c > > index 4bfcad7..e30fe6e 100644 > > --- a/gcc/cgraphunit.c > > +++ b/gcc/cgraphunit.c > > @@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind > > ORDER_UNDEFINED = 0, > > ORDER_FUNCTION, > > ORDER_VAR, > > + ORDER_VAR_UNDEF, > > ORDER_ASM > > }; > > > > @@ -2187,16 +2188,20 @@ output_in_order (bool no_reorder) > > } > > } > > > > - FOR_EACH_DEFINED_VARIABLE (pv) > > - if (!DECL_EXTERNAL (pv->decl)) > > - { > > - if (no_reorder && !pv->no_reorder) > > - continue; > > - i = pv->order; > > - gcc_assert (nodes[i].kind == ORDER_UNDEFINED); > > - nodes[i].kind = ORDER_VAR; > > - nodes[i].u.v = pv; > > - } > > + /* There is a similar loop in symbol_table::output_variables. > > + Please keep them in sync. */ > > + FOR_EACH_VARIABLE (pv) > > + { > > + if (no_reorder && !pv->no_reorder) > > + continue; > > + if (DECL_HARD_REGISTER (pv->decl) > > + || DECL_HAS_VALUE_EXPR_P (pv->decl)) > > + continue; > > + i = pv->order; > > + gcc_assert (nodes[i].kind == ORDER_UNDEFINED); > > + nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF; > > + nodes[i].u.v = pv; > > + } > > > > for (pa = symtab->first_asm_symbol (); pa; pa = pa->next) > > { > > @@ -2222,16 +2227,13 @@ output_in_order (bool no_reorder) > > break; > > > > case ORDER_VAR: > > -#ifdef ACCEL_COMPILER > > - /* Do not assemble "omp declare target link" vars. */ > > - if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl) > > - && lookup_attribute ("omp declare target link", > > - DECL_ATTRIBUTES (nodes[i].u.v->decl))) > > - break; > > -#endif > > nodes[i].u.v->assemble_decl (); > > break; > > > > + case ORDER_VAR_UNDEF: > > + assemble_undefined_decl (nodes[i].u.v->decl); > > + break; > > + > > case ORDER_ASM: > > assemble_asm (nodes[i].u.a->asm_str); > > break; > > diff --git a/gcc/varpool.c b/gcc/varpool.c > > index ab615fa..e5f991e 100644 > > --- a/gcc/varpool.c > > +++ b/gcc/varpool.c > > @@ -733,11 +733,6 @@ symbol_table::output_variables (void) > > > > timevar_push (TV_VAROUT); > > > > - FOR_EACH_VARIABLE (node) > > - if (!node->definition > > - && !DECL_HAS_VALUE_EXPR_P (node->decl) > > - && !DECL_HARD_REGISTER (node->decl)) > > - assemble_undefined_decl (node->decl); > > FOR_EACH_DEFINED_VARIABLE (node) > > { > > /* Handled in output_in_order. */ > > @@ -747,20 +742,19 @@ symbol_table::output_variables (void) > > node->finalize_named_section_flags (); > > } > > > > - FOR_EACH_DEFINED_VARIABLE (node) > > + /* There is a similar loop in output_in_order. Please keep them in sync. */ > > + FOR_EACH_VARIABLE (node) > > { > > /* Handled in output_in_order. */ > > if (node->no_reorder) > > continue; > > -#ifdef ACCEL_COMPILER > > - /* Do not assemble "omp declare target link" vars. */ > > - if (DECL_HAS_VALUE_EXPR_P (node->decl) > > - && lookup_attribute ("omp declare target link", > > - DECL_ATTRIBUTES (node->decl))) > > + if (DECL_HARD_REGISTER (node->decl) > > + || DECL_HAS_VALUE_EXPR_P (node->decl)) > > continue; > > -#endif > > - if (node->assemble_decl ()) > > - changed = true; > > + if (node->definition) > > + changed |= node->assemble_decl (); > > + else > > + assemble_undefined_decl (node->decl); > > } > > timevar_pop (TV_VAROUT); > > return changed; > > > >
On 06/23/2016 08:45 AM, Alexander Monakov wrote: > Hi, > > I've discovered that this assert in my patch was too restrictive: > > + if (DECL_HAS_VALUE_EXPR_P (pv->decl)) > + { > + gcc_checking_assert (lookup_attribute ("omp declare target link", > + DECL_ATTRIBUTES (pv->decl))); > > Testing for the nvptx target uncovered that there's another case where a > global variable would have a value expr: emutls. Sorry for not spotting it > earlier (but at least the new assert did its job). I think we should always > skip here over decls that have value-exprs, just like hard-reg vars are > skipped. The following patch does that. Is this still OK? > > (bootstrapped/regtested on x86-64) > > Alexander > > * cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF. > (output_in_order): Loop over undefined variables too. Output them > via assemble_undefined_decl. Skip variables that correspond to hard > registers or have value-exprs. > * varpool.c (symbol_table::output_variables): Handle undefined > variables together with defined ones. I haven't followed this issue at all. So bear with me if I ask a question you've already answered. Is the point here to be able to deduce what symbols are external & undefined and emit some kind of directive to the assembler in that case? Avoiding duplicates as well as symbols which may have had originally looked like external & undefined, but later we find a definition? The reason I ask someone might be able to simplify a bit of the PA backend if that's the goal here. The PA (when using the SOM object format) has similar needs. We solved it by queuing up everything that might need "importing". Then at the end of the compilation unit, we'd walk that queue of symbols emitting the proper magic. Jeff
On Thu, 14 Jul 2016, Jeff Law wrote: > Is the point here to be able to deduce what symbols are external & undefined > and emit some kind of directive to the assembler in that case? Yes, PTX assembly requires that properly typed declarations are emitted for external references. Today, the NVPTX backend in GCC performs that internally for function symbols, and relies on the middle-end to do that for data symbols, but when the port was added that was implemented only when -ftoplevel-reorder is in effect (the default). The point of my patch is to handle it under -fno-toplevel-reorder in the same way. > Avoiding duplicates this is not required for PTX, but nevertheless a good cleanup we get for free > as well as symbols which may have had originally looked like external & > undefined, but later we find a definition? I think this only would be a problem if GCC continued to support -fno-unit-at-a-time. AFAIK today GCC's symtab reflects symbol availability exactly, because the whole translation unit has been read upfront. > The reason I ask someone might be able to simplify a bit of the PA backend if > that's the goal here. The PA (when using the SOM object format) has similar > needs. We solved it by queuing up everything that might need "importing". > Then at the end of the compilation unit, we'd walk that queue of symbols > emitting the proper magic. *nod*, although as I mentioned above today it's handled in a generic way only for undefined data symbols, not undefined function symbols. Alexander
On 07/14/2016 12:33 PM, Alexander Monakov wrote: > On Thu, 14 Jul 2016, Jeff Law wrote: >> Is the point here to be able to deduce what symbols are external & undefined >> and emit some kind of directive to the assembler in that case? > > Yes, PTX assembly requires that properly typed declarations are emitted for > external references. Today, the NVPTX backend in GCC performs that internally > for function symbols, and relies on the middle-end to do that for data symbols, > but when the port was added that was implemented only when -ftoplevel-reorder is > in effect (the default). Right. I remember noting that in my brief review of the PTX assembly specs and figuring it wouldn't be a big deal since we had a functional hack in place for the PA/SOM which has similar requirements. > > *nod*, although as I mentioned above today it's handled in a generic way only > for undefined data symbols, not undefined function symbols. Understood. Quickly reviewing the PA code, the other bit of magic is we avoided importing a symbol which was declared as extern, but never actually used. IIRC that caused the HP/SOM linker to core dump. Hopefully the PTX assembler/linker/finalizer handles that case more gracefully... Jeff
On 06/23/2016 08:45 AM, Alexander Monakov wrote: > Hi, > > I've discovered that this assert in my patch was too restrictive: > > + if (DECL_HAS_VALUE_EXPR_P (pv->decl)) > + { > + gcc_checking_assert (lookup_attribute ("omp declare target link", > + DECL_ATTRIBUTES (pv->decl))); > > Testing for the nvptx target uncovered that there's another case where a > global variable would have a value expr: emutls. Sorry for not spotting it > earlier (but at least the new assert did its job). I think we should always > skip here over decls that have value-exprs, just like hard-reg vars are > skipped. The following patch does that. Is this still OK? > > (bootstrapped/regtested on x86-64) > > Alexander > > * cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF. > (output_in_order): Loop over undefined variables too. Output them > via assemble_undefined_decl. Skip variables that correspond to hard > registers or have value-exprs. > * varpool.c (symbol_table::output_variables): Handle undefined > variables together with defined ones. OK. Thanks for your patience. jeff
diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c index 4bfcad7..e30fe6e 100644 --- a/gcc/cgraphunit.c +++ b/gcc/cgraphunit.c @@ -2141,6 +2141,7 @@ enum cgraph_order_sort_kind ORDER_UNDEFINED = 0, ORDER_FUNCTION, ORDER_VAR, + ORDER_VAR_UNDEF, ORDER_ASM }; @@ -2187,16 +2188,20 @@ output_in_order (bool no_reorder) } } - FOR_EACH_DEFINED_VARIABLE (pv) - if (!DECL_EXTERNAL (pv->decl)) - { - if (no_reorder && !pv->no_reorder) - continue; - i = pv->order; - gcc_assert (nodes[i].kind == ORDER_UNDEFINED); - nodes[i].kind = ORDER_VAR; - nodes[i].u.v = pv; - } + /* There is a similar loop in symbol_table::output_variables. + Please keep them in sync. */ + FOR_EACH_VARIABLE (pv) + { + if (no_reorder && !pv->no_reorder) + continue; + if (DECL_HARD_REGISTER (pv->decl) + || DECL_HAS_VALUE_EXPR_P (pv->decl)) + continue; + i = pv->order; + gcc_assert (nodes[i].kind == ORDER_UNDEFINED); + nodes[i].kind = pv->definition ? ORDER_VAR : ORDER_VAR_UNDEF; + nodes[i].u.v = pv; + } for (pa = symtab->first_asm_symbol (); pa; pa = pa->next) { @@ -2222,16 +2227,13 @@ output_in_order (bool no_reorder) break; case ORDER_VAR: -#ifdef ACCEL_COMPILER - /* Do not assemble "omp declare target link" vars. */ - if (DECL_HAS_VALUE_EXPR_P (nodes[i].u.v->decl) - && lookup_attribute ("omp declare target link", - DECL_ATTRIBUTES (nodes[i].u.v->decl))) - break; -#endif nodes[i].u.v->assemble_decl (); break; + case ORDER_VAR_UNDEF: + assemble_undefined_decl (nodes[i].u.v->decl); + break; + case ORDER_ASM: assemble_asm (nodes[i].u.a->asm_str); break; diff --git a/gcc/varpool.c b/gcc/varpool.c index ab615fa..e5f991e 100644 --- a/gcc/varpool.c +++ b/gcc/varpool.c @@ -733,11 +733,6 @@ symbol_table::output_variables (void) timevar_push (TV_VAROUT); - FOR_EACH_VARIABLE (node) - if (!node->definition - && !DECL_HAS_VALUE_EXPR_P (node->decl) - && !DECL_HARD_REGISTER (node->decl)) - assemble_undefined_decl (node->decl); FOR_EACH_DEFINED_VARIABLE (node) { /* Handled in output_in_order. */ @@ -747,20 +742,19 @@ symbol_table::output_variables (void) node->finalize_named_section_flags (); } - FOR_EACH_DEFINED_VARIABLE (node) + /* There is a similar loop in output_in_order. Please keep them in sync. */ + FOR_EACH_VARIABLE (node) { /* Handled in output_in_order. */ if (node->no_reorder) continue; -#ifdef ACCEL_COMPILER - /* Do not assemble "omp declare target link" vars. */ - if (DECL_HAS_VALUE_EXPR_P (node->decl) - && lookup_attribute ("omp declare target link", - DECL_ATTRIBUTES (node->decl))) + if (DECL_HARD_REGISTER (node->decl) + || DECL_HAS_VALUE_EXPR_P (node->decl)) continue; -#endif - if (node->assemble_decl ()) - changed = true; + if (node->definition) + changed |= node->assemble_decl (); + else + assemble_undefined_decl (node->decl); } timevar_pop (TV_VAROUT); return changed;
Hi, I've discovered that this assert in my patch was too restrictive: + if (DECL_HAS_VALUE_EXPR_P (pv->decl)) + { + gcc_checking_assert (lookup_attribute ("omp declare target link", + DECL_ATTRIBUTES (pv->decl))); Testing for the nvptx target uncovered that there's another case where a global variable would have a value expr: emutls. Sorry for not spotting it earlier (but at least the new assert did its job). I think we should always skip here over decls that have value-exprs, just like hard-reg vars are skipped. The following patch does that. Is this still OK? (bootstrapped/regtested on x86-64) Alexander * cgraphunit.c (cgraph_order_sort_kind): New entry ORDER_VAR_UNDEF. (output_in_order): Loop over undefined variables too. Output them via assemble_undefined_decl. Skip variables that correspond to hard registers or have value-exprs. * varpool.c (symbol_table::output_variables): Handle undefined variables together with defined ones.