Gautam, Sorry I am going to pass the ball back to you. I don't know much about gcc compatibility, so my review is not going to be meaningful. I do support your concern about basing "name" as having specific meaning (names with special character is the last resort for internal names, lacking any other way) and has already discuss with Gang in private. Sun
On Mon, Apr 23, 2012 at 1:29 AM, Gautam Chakrabarti <gautam.c...@yahoo.com> wrote: > I am not sure if the comments below address my questions below. > > As you say below, you don't know either what a variable not having a name > means. I don't know either, as I said below. That is exactly the concern > with this change. I don't think it has any well-defined meaning --- because > a variable may or may not have a name --- nothing should depend on > that. Most TREE fields have a well-defined meaning (from GCC code), and that > is what we depend on while converting to WHIRL. Relevant fields in this > context are TREE_PUBLIC, DECL_CONTEXT, ..., and the existing wgen code > depends on the semantics of these fields. > > Also it shouldn't be a big deal to add a name in GCC to such temp variables, > even one Open64 developer could go and make a fix to GCC code to add a name > to temp variables. > > So I have tried to explain my reservations, and haven't seen any convincing > reply. > > I will let Sun decide on this review. > > Thanks, > Gautam > > From: Gang Yu <yugang...@gmail.com> > To: Gautam Chakrabarti <gautam.c...@yahoo.com> > Cc: Sun Chan <sun.c...@gmail.com>; open64-devel > <open64-devel@lists.sourceforge.net> > Sent: Sunday, April 22, 2012 6:19 AM > > Subject: Re: [Open64-devel] review request for fix bug965[WGEN] > > My comments embeded. > > On Sun, Apr 22, 2012 at 3:18 AM, Gautam Chakrabarti <gautam.c...@yahoo.com> > wrote: > > GCC could potentially use the same (temp) variable in multiple PUs and it > still would have worked. > > I am not sure what is the context on that. A temp variable is generated > inside a PU, it represents the temporary computation. If it is really used > accross PUs, it means compiler "smartly" introduces implicit sharing among > different PUs, i.e, it introduce holes to the PU, I don't think this is an > intended design. > > > > 1. what happens if GCC suddenly decides to actually give a name to these > temp variables. that would still be valid TREE IR, right? But with that GCC > IR, this change will stop doing the intended thing, and this variable will > again show up in object file even if unused. > > > And you previously mentioned. >>>> Is it defined anywhere what a variable not having a name exactly means? >>>> I don't know of any specification of this. > I don't know the specification either. And AFAIK, GCC's document on this, > "GNU compiler collection Internals" is only specified data structures on the > generic IR for front-end contributors. Things like to whether to generate > "NULL" name tree for temp computation are just GCC internal behaviors. They > don't specify these, so we can't predicate what GCC will change the "IR". > Even if we change to a new gcc front-end, we still have to study the gcc > dump first. > > > > 2. what happens also if GCC actually decides to NOT give a name to a real > file-scope variable? That would also be legal, right? But then this change > will trigger and attempt to make it local to a PU. > > > As previosly explained, this is still a guess on GCC internal behavior. But, > I still don't think GCC will decide to eliminate the name to a variable in a > user program. Is it intended to change the semantics for the program? > > In general, we are all unsure on the specific meanings on the GCC dump tree. > But we respect gatekeepers' experience and judgement, we may fix this bug on > another approach. > > Thanks > > Regards > Gang > > > > From: Gang Yu <yugang...@gmail.com> > To: Gautam Chakrabarti <gautam.c...@yahoo.com> > Cc: Sun Chan <sun.c...@gmail.com>; open64-devel > <open64-devel@lists.sourceforge.net> > Sent: Saturday, April 21, 2012 3:53 AM > > Subject: Re: [Open64-devel] review request for fix bug965[WGEN] > > Hi, Gautam: > > Thanks for the detail thoughts. My comments embedded. > > On Sat, Apr 21, 2012 at 4:46 PM, Gautam Chakrabarti <gautam.c...@yahoo.com> > wrote: > > Hi Gang, > > Thanks for the explanations. The concern I have with this fix is the > following: > > What the fix does is if the compiler has named a variable to be anon*, then > it allocates it to the local symtab as AUTO. Else, it takes the usual path > of allocating it to the global symtab as FSTATIC. > > In turn the compiler names a variable as anon* when it does not have a name. > This implies, what the fix does is when a variable does not have a name, > allocate it to local symtab. > > The question is exactly when a variable may not have a name in GCC IR. Do we > understand exactly when this can happen? Because that is when this change > allocates it to local symtab. > > > > If we have a look from another angle, i.e, from WGEN. In Create_ST_For_Tree, > we generate a ST for a tree with spin NULL name, we define a WGEN > protocol name for this NULL var. We can make sure this name can only be > referenced inside current PU. otherwise, we can say the compiler generate > reference to anonymous(internal) vars in other PUs. If so, I am not sure and > can not understand what is the context. That's why I made such a change > proposal. > > > > > Another issue is I believe the IF condition that has all this code looks > like this: > > if (gs_decl_context (decl_node) == 0 || > gs_tree_code (gs_decl_context (decl_node)) == > GS_NAMESPACE_DECL || > gs_tree_code (gs_decl_context (decl_node)) == GS_RECORD_TYPE ) > { > > So I am assuming for this anon variable, one of these is true. I am assuming > that actually gs_decl_context(decl_node) is NULL (let me know if this is not > correct). > > > Yes, it is NULL. > > > This is the reason we put it as FSTATIC in the first place, because this is > how DECL_CONTEXT is defined: > > ----- > For VAR_DECL, > PARM_DECL, FUNCTION_DECL, LABEL_DECL, and CONST_DECL nodes, this > points to either the FUNCTION_DECL for the containing function, > the RECORD_TYPE or UNION_TYPE for the containing type, or > NULL_TREE or a TRANSLATION_UNIT_DECL if the given decl has "file > scope" > ----- > > > > Thanks, I see above comments in wgen_dst.cxx. could you please kindly point > me to the GCC documents around? Thanks > > > > So for some reason GCC IR is telling wgen that this variable belongs in file > scope. So we might need to be more careful before allocating it as AUTO. > > I think the fix is close to correct. But we need to make this decision based > on something else, not that it has a NULL name, unless this is already > documented somewhere. > > It could be that this var has a special flag that would indicate it's safe > to do this. If not, one could study if GCC initially puts it in file-scope, > how can it finally remove it. If nothing else works, one approach might be > to specially mark these variables in GCC and use that in wgen. > > Could you give the dump of the tree for this variable? > > GS_VAR_DECL<29280> { (0)GS_TCC<29456> GS_TCC_DECLARATION > (3)IB_BIT_VECTOR<29460> GS_TREE_USED GS_DECL_UNSIGNED GS_DECL_IGNORED_P > GS_DECL_ARTIFICIAL (5)IB_STRING<29484> "DI" (6)IB_STRING<29492> "anon.c" > (7)IB_INT<29500> 7 (24)IB_INT<29476> 1888 (25)IB_INT<29508> 8 > (29)IB_BIT_VECTOR<29516> (42)IB_BIT_VECTOR<29532> > @gs_decl_size GS_INTEGER_CST<1168> { (0)GS_TCC<1200> GS_TCC_CONSTANT > (3)IB_BIT_VECTOR<1204> GS_TREE_CONSTANT GS_TREE_INVARIANT > (4)IB_UNSIGNED_LONG_LONG<2668> 64 (5)IB_LONG_LONG<2684> 0 > } > } > > > > > > -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