Thank you very much, Sun.
This is a principle teach, thanks. I suggest we add the following rule to
the coding style.
Be careful to compiler generated symbols. In any case, the compiler
generated names should not be in the namespace of user programs. And it is
better to easy read the usage for the compiler temporary name only by the
name itself.
The new patch below:
osprey/wgen/wgen_spin_symbol.cxx -- ff56fe0..70b199d 100644
--- a/osprey/wgen/wgen_spin_symbol.cxx
+++ b/osprey/wgen/wgen_spin_symbol.cxx
@@ -88,6 +88,8 @@ extern "C"{
//#include "tree_cmp.h"
#include <erglob.h>
#include <ext/hash_set>
+#define TVAR_PREFIX ".anon_"
+#define TVAR_PREFIX_LEN (sizeof(TVAR_PREFIX) - 1)
using __gnu_cxx::hash_set;
typedef struct {
size_t operator()(void* p) const { return reinterpret_cast<size_t>(p);
}
@@ -1607,7 +1609,7 @@ Create_ST_For_Tree (gs_t decl_node)
else if (gs_decl_name (decl_node))
name = (char *) gs_identifier_pointer (gs_decl_name (decl_node));
else {
- sprintf(tempname, "anon%d", ++anon_count);
+ sprintf(tempname, TVAR_PREFIX "%d", ++anon_count);
name = tempname;
}
@@ -1774,10 +1776,18 @@ Create_ST_For_Tree (gs_t decl_node)
eclass = EXPORT_PREEMPTIBLE;
}
else {
+ if (name != tempname || strncmp(tempname, TVAR_PREFIX,
TVAR_PREFIX_LEN)) {
sclass = SCLASS_FSTATIC;
eclass = EXPORT_LOCAL;
+ } else {
+ sclass = SCLASS_AUTO;
+ eclass = EXPORT_LOCAL;
+ }
}
- level = GLOBAL_SYMTAB;
+ if (name != tempname || strncmp(tempname, TVAR_PREFIX,
TVAR_PREFIX_LEN))
+ level = GLOBAL_SYMTAB;
+ else
+ level = CURRENT_SYMTAB;
}
else {
#ifdef KEY
Why I use the prefix ".anon_"
*) look at the wgen_spin_symbol.cxx, it's common to use "." prefix to
indicate compiler generated symbols
*) I still found
197static char*
198Get_Name (gs_t node)
199{
200 static UINT anon_num = 0;
201 static char buf[64];
202
203 if (node == NULL) {
204 ++anon_num;
205 sprintf(buf, ".anonymous.%d", anon_num);
206 return buf;
207 }
this is for generation of anonymous names for the tree, and we should be
able to discriminate.
Regards
Gang
On Fri, Apr 20, 2012 at 7:24 PM, Sun Chan <sun.c...@gmail.com> wrote:
> this is my suggestion:
> 1. find one special char that gas can accept but not part of
> C/C++/Fortran (note that it could be lang specific), say this is $
> 2. use anon$ instead of anon
> 3. #define this string in config.h or something appropriate
> 4. replace all hard coded use of "anon"
> then your change is the right fix, not continue previous bad practice.
>
> Sun
>
> On Fri, Apr 20, 2012 at 3:05 PM, Gang Yu <yugang...@gmail.com> wrote:
> > On Fri, Apr 20, 2012 at 2:38 PM, Sun Chan <sun.c...@gmail.com> wrote:
> >>
> >> It is one thing to allow FE to generate temps with special substring.
> >> It it another to assume that special substring always mean compiler
> >> generated temps. They are not logically equivalent.
> >> Sun
> >
> >
> > Maybe I do not make things clear in my last post. Now, I will put more
> > details.
> >
> > The source code in wgen_spin_symbol.cxx
> >
> > 1569ST*
> > 1570Create_ST_For_Tree (gs_t decl_node)
> > 1571{
> > 1572 TY_IDX ty_idx;
> > 1573 ST* st = NULL;
> > 1574 char *name;
> > 1575 char tempname[32];
> > 1595 if (gs_tree_code(decl_node) == GS_RESULT_DECL) {
> > 1596 sprintf(tempname, ".result_decl_%d", gs_decl_uid(decl_node));
> > 1597 name = tempname;
> > 1598 }
> > 1599 else if ((gs_tree_code(decl_node) == GS_FUNCTION_DECL ||
> > 1600 gs_tree_code(decl_node) == GS_PARM_DECL ||
> > 1601 (gs_tree_code(decl_node) == GS_VAR_DECL &&
> > 1602 gs_decl_asmreg(decl_node) >= 0)) &&
> > 1603 gs_decl_name(decl_node) != 0)
> > 1604 name = (char *) gs_identifier_pointer (gs_decl_name (decl_node));
> > 1605 else if (gs_decl_assembler_name (decl_node) && gs_decl_name
> > (decl_node))
> > 1606 name = (char *) gs_identifier_pointer (gs_decl_assembler_name
> > (decl_node));
> > 1607 else if (gs_decl_name (decl_node))
> > 1608 name = (char *) gs_identifier_pointer (gs_decl_name (decl_node));
> >
> > 1609 else {
> > 1610 sprintf(tempname, "anon%d", ++anon_count);
> > 1611 name = tempname;
> > 1612 }
> > ...
> >
> > when control goes here, we could say the compiler generate temp var in
> two
> > cases. line 1595-1598 and line 1609-1612, i.e, the green part. common
> part
> > "name = tempname" can be used as the indicate.
> >
> > We just only want to set the temp var to SCLASS_AUTO for those generated
> in
> > "else" part, i.e, 1609-1612, these names contains "anon", without other
> flag
> > to indicate, we just use this name prefix. So, we can have the following
> > change:
> >
> > 1777 if (name != tempname || strncmp(tempname,"anon",4)) {
> //
> > Not a temp var, or temp var for result_decl, no change
> > 1778 sclass = SCLASS_FSTATIC;
> > 1779 eclass = EXPORT_LOCAL;
> > 1780 } else { // Yes, these are temp vars we want to change
> > 1781 sclass = SCLASS_AUTO;
> > 1782 eclass = EXPORT_LOCAL;
> > 1783 }
> > 1784 }
> > 1785 if (name != tempname || strncmp(tempname,"anon",4))
> > 1786 level = GLOBAL_SYMTAB;
> > 1787 else
> > 1788 level = CURRENT_SYMTAB;
> >
> > Regards
> > Gang
> >
> >>
> >> On Fri, Apr 20, 2012 at 2:01 PM, Gang Yu <yugang...@gmail.com> wrote:
> >> > Hi, Gautam
> >> >
> >> > Thanks for the review.
> >> >
> >> > You post a good questtion. In my patch, you may note
> >> >
> >> > + if (name != tempname || strncmp(tempname,"anon",4)) {
> >> >
> >> > "name != tempname" address your care.
> >> >
> >> > Walk though the wgen_spin_symbol.cxx:1611, you may note that
> >> >
> >> > 1609 else {
> >> > 1610 sprintf(tempname, "anon%d", ++anon_count);
> >> > 1611 name = tempname;
> >> > 1612 }
> >> > name == tempname indicate that this is FE generated temp var rather
> >> > than var
> >> > in user program.
> >> > Regards
> >> > Gang
> >> >
> >> >
> >> > On Fri, Apr 20, 2012 at 1:54 PM, Gautam Chakrabarti
> >> > <gautam.c...@yahoo.com>
> >> > wrote:
> >> >>
> >> >> Hi,
> >> >>
> >> >> Thanks for working on this issue. But this change is not clean. The
> >> >> change
> >> >> is essentially assuming special semantics based on a variable name.
> >> >> What if
> >> >> a user variable is named anon?
> >> >>
> >> >> -Gautam
> >> >>
> >> >> From: Gang Yu <yugang...@gmail.com>
> >> >> To: open64-devel <open64-devel@lists.sourceforge.net>
> >> >> Sent: Thursday, April 19, 2012 2:01 AM
> >> >> Subject: [Open64-devel] review request for fix bug965[WGEN]
> >> >>
> >> >> Hi,
> >> >>
> >> >> Could a gatekeeper please help review the fix for
> >> >> bug965?(http://bugs.open64.net/show_bug.cgi?id=965)
> >> >>
> >> >> This is a code-size issue for open64, i.e, the compiler generate
> >> >> garbage
> >> >> code although it still make it right.
> >> >>
> >> >> a cut-down case below:
> >> >>
> >> >> static inline __attribute__((always_inline)) int
> >> >> x86_this_cpu_constant_test_bit(unsigned int nr,
> >> >> const unsigned long *addr)
> >> >> {
> >> >> unsigned long *a = (unsigned long *)addr + nr / 64;
> >> >>
> >> >> return ((1UL << (nr % 64)) & ({ typeof(*a) pfo_ret__; switch
> >> >> (sizeof(*a))
> >> >> {
> >> >> case 1: asm("mov" "b ""%%""gs"":" "%P" "1"",%0" : "=q" (pfo_ret_\
> >> >> _) : "m" (*a)); break; case 2: asm("mov" "w ""%%""gs"":" "%P"
> "1"",%0"
> >> >> :
> >> >> "=r"
> >> >> (pfo_ret__) : "m" (*a)); break; case 4: asm("mov" "l ""%%""gs""\
> >> >> :" "%P" "1"",%0" : "=r" (pfo_ret__) : "m" (*a)); break; case 8:
> >> >> asm("mov"
> >> >> "q
> >> >> ""%%""gs"":" "%P" "1"",%0" : "=r" (pfo_ret__) : "m" (*a)); break\
> >> >> ; default: __bad_percpu_size(); } pfo_ret__; })) != 0;
> >> >> }
> >> >> opencc -c anon.c -keep
> >> >> yug@jupiter:~/work/codesize> readelf -s anon.o
> >> >>
> >> >>
> >> >> Symbol table '.symtab' contains 8 entries:
> >> >> Num: Value Size Type Bind Vis Ndx Name
> >> >> 4: 0000000000000000 8 OBJECT LOCAL DEFAULT 3 anon1
> >> >> x86_this_cpu_constant_test_bit is non-called inline keyword function,
> >> >> so
> >> >> there should generate nothing symbols in this object file, however,
> >> >> anon1 appears unexpected.
> >> >> Analysis:
> >> >>
> >> >> When we examine the WGEN code, we found that the code snipper like
> >> >> these:
> >> >>
> >> >> 1569ST*
> >> >> 1570Create_ST_For_Tree (gs_t decl_node)
> >> >> 1571{
> >> >> ...
> >> >> 1609 else {
> >> >> 1610 sprintf(tempname, "anon%d", ++anon_count);
> >> >> 1611 name = tempname;
> >> >> 1612 }
> >> >> ...
> >> >> 1750 if (gs_tree_public (decl_node)) {
> >> >> ...
> >> >> 1776 else {
> >> >> 1777 sclass = SCLASS_FSTATIC;
> >> >> 1778 eclass = EXPORT_LOCAL;
> >> >> 1779 }
> >> >> 1780 level = GLOBAL_SYMTAB;
> >> >> 1781 }
> >> >>
> >> >> this is to say, when current gs_tree is not public, we generate such
> a
> >> >> variable as File-Scope(static) global variable.
> >> >> However, anon1 appears as the temporary variable in a computation.
> >> >>
> >> >> U8U8LDID 0 <2,4,pfo_ret__> T<9,.predef_U8,8>
> >> >> U8STID 0 <1,51,anon1> T<9,.predef_U8,8> {line: 1/7}
> >> >> U8INTCONST 1 (0x1)
> >> >> I4I4LDID 0 <2,1,nr> T<4,.predef_I4,4>
> >> >> I4INTCONST 63 (0x3f)
> >> >> I4BAND
> >> >> U8SHL
> >> >> U8U8LDID 0 <1,51,anon1> T<9,.predef_U8,8>
> >> >> U8BAND
> >> >> U8INTCONST 0 (0x0)
> >> >> I4U8NE
> >> >> I4RETURN_VAL {line: 0/0}
> >> >> and we do find the producer for anon
> >> >> 1609 else {
> >> >> 1610 sprintf(tempname, "anon%d", ++anon_count);
> >> >> 1611 name = tempname;
> >> >> 1612 }
> >> >> we should make it SCLASS_AUTO in LOCAL_SYMTAB.
> >> >>
> >> >> Suggested patch:
> >> >> osprey/wgen/wgen_spin_symbol.cxx -- ff56fe0..b24a800 100644
> >> >> --- a/osprey/wgen/wgen_spin_symbol.cxx
> >> >> +++ b/osprey/wgen/wgen_spin_symbol.cxx
> >> >> @@ -1774,10 +1774,18 @@ Create_ST_For_Tree (gs_t decl_node)
> >> >> eclass = EXPORT_PREEMPTIBLE;
> >> >> }
> >> >> else {
> >> >> + if (name != tempname || strncmp(tempname,"anon",4)) {
> >> >> sclass = SCLASS_FSTATIC;
> >> >> eclass = EXPORT_LOCAL;
> >> >> + } else {
> >> >> + sclass = SCLASS_AUTO;
> >> >> + eclass = EXPORT_LOCAL;
> >> >> + }
> >> >> }
> >> >> - level = GLOBAL_SYMTAB;
> >> >> + if (name != tempname || strncmp(tempname,"anon",4))
> >> >> + level = GLOBAL_SYMTAB;
> >> >> + else
> >> >> + level = CURRENT_SYMTAB;
> >> >> }
> >> >> else {
> >> >> #ifdef KEY
> >> >>
> >> >> As an example, I show the codesize comparison to gcc generation and
> the
> >> >> source code in the attach, we can see this is really bad for
> code-size.
> >> >> Could a gatekeeper please help a review? thanks in advance.
> >> >>
> >> >>
> >> >> Regards
> >> >> Gang
> >> >>
> >> >>
> >> >>
> >> >>
> ------------------------------------------------------------------------------
> >> >> For Developers, A Lot Can Happen In A Second.
> >> >> Boundary is the first to Know...and Tell You.
> >> >> Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
> >> >> http://p.sf.net/sfu/Boundary-d2dvs2
> >> >> _______________________________________________
> >> >> Open64-devel mailing list
> >> >> Open64-devel@lists.sourceforge.net
> >> >> https://lists.sourceforge.net/lists/listinfo/open64-devel
> >> >>
> >> >>
> >> >
> >> >
> >> >
> >> >
> ------------------------------------------------------------------------------
> >> > For Developers, A Lot Can Happen In A Second.
> >> > Boundary is the first to Know...and Tell You.
> >> > Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
> >> > http://p.sf.net/sfu/Boundary-d2dvs2
> >> > _______________________________________________
> >> > Open64-devel mailing list
> >> > Open64-devel@lists.sourceforge.net
> >> > https://lists.sourceforge.net/lists/listinfo/open64-devel
> >> >
> >
> >
>
------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
Open64-devel mailing list
Open64-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/open64-devel
