Thanks for the explanations, Gang. Could you please hold off on this checkin? I
want to take another look at it before this is checked in.
-Gautam
________________________________
From: Sun Chan <sun.c...@gmail.com>
To: Gang Yu <yugang...@gmail.com>
Cc: Gautam Chakrabarti <gautam.c...@yahoo.com>; open64-devel
<open64-devel@lists.sourceforge.net>
Sent: Friday, April 20, 2012 6:39 PM
Subject: Re: [Open64-devel] review request for fix bug965[WGEN]
can the stmts
#define TVAR_PREFIX ".anon_"
#define TVAR_PREFIX_LEN (sizeof(TVAR_PREFIX) - 1)
be inside some .h file (such as defs.h)?
Sun
On Sat, Apr 21, 2012 at 9:23 AM, Gang Yu <yugang...@gmail.com> wrote:
> 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
