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
