someone sent me the following comment to the code which I think is reasonable, pls address that: Thx! Sun
All comments are based on his tarball which had patch 0003-Cleanup-64-bit-compilation-issues-in-FFE.patch ----------------- // This is confusing // It's cosmetic and I assume to make the code more readable, but that fails for me -# define Is_Target_32bit() (cmd_line_flags.s_pointer8 == 0) -# define Is_Target_64bit() (cmd_line_flags.s_pointer8 == 1) +# define FFE_Is_Target_32bit() (cmd_line_flags.s_pointer8 == 0) +# define FFE_Is_Target_64bit() (cmd_line_flags.s_pointer8 == 1) // How many bits are needed -# define AT_OBJ_NAME_LONG(IDX) ((long *)&name_pool[AT_NAME_IDX(IDX)]) +# define AT_OBJ_NAME_WORD(IDX) ((name_pool_word_type *)&name_pool[AT_NAME_IDX(IDX)]) // Their opening comment - yet they fail to grok that it doesn't help the situation long term - see below Note that the above typedefs use int instead of long, since this keeps the indices of table entries the same between the 32 and 64 bit builds of the compiler when compiling the same code. // PLEASE STOP USING NATIVE TYPES // int32_t or something *really* should be used here // it's also very confusing to me that it's called WORD_SIZE, but being defined an int // If they are touching the code - why not correct the name for accuracy as well -#define FMT_ENTRY_WORD_SIZE (sizeof(fmt_type)/sizeof(long)) +#define FMT_ENTRY_WORD_SIZE (sizeof(fmt_type)/sizeof(int)) On Tue, Aug 14, 2012 at 5:22 AM, Gilmore, Doug <doug.gilm...@amd.com> wrote: > Per David's message, he already reviewed the change so if there are not > any objections I'll commit the change in the next few days. > > Doug > -----Original Message----- > From: David Coakley [mailto:dcoak...@gmail.com] > Sent: Monday, August 13, 2012 2:15 PM > To: Gilmore, Doug > Subject: Re: FW: [Open64-devel] Review request: fixing issues when building > the compiler as 64-bit binaries > > Hi Doug, > > Yes, I reviewed that one. I understood that this code was already in > the tree and your patch just made it easier to get to by adding a > configure option. I don't see any problem with that. > > -David > > On Sat, Aug 11, 2012 at 9:57 PM, Gilmore, Doug <doug.gilm...@amd.com> wrote: >> Hi David, >> >> You already reviewed this change right? >> >> Doug >> >> -----Original Message----- >> From: Sun Chan [mailto:sun.c...@gmail.com] >> Sent: Friday, August 10, 2012 5:14 PM >> To: Gilmore, Doug >> Cc: Jian-Xin Lai; open64-devel >> Subject: Re: [Open64-devel] Review request: fixing issues when building the >> compiler as 64-bit binaries >> >> someone still need to code review, I thought what we agreed is that >> the additional id is useful >> Sun >> >> On Sat, Aug 11, 2012 at 7:38 AM, Gilmore, Doug <doug.gilm...@amd.com> wrote: >>> The field should only be referenced in debugging output. >>> >>> Along with Jian-Xin's use case, it is also useful tracking >>> down the source of WHIRL differences when building with >>> different builds of the compiler 32/64-bit and/or >>> release/debug. >>> >>> Note that functionality is already there, but you have to >>> change the source to enable it, which makes your source tree >>> less useful. However if it can configure the capability you >>> can still have can have a build that is compatible with >>> other builds of the compiler and another build with the ID >>> enabled where you can more easily track down problems. >>> >>> Are there still objections to committing this patch? >>> >>> Doug >>>> -----Original Message----- >>>> From: Sun Chan [mailto:sun.c...@gmail.com] >>>> Sent: Thursday, August 02, 2012 11:15 PM >>>> To: Jian-Xin Lai >>>> Cc: Gilmore, Doug; open64-devel >>>> Subject: Re: [Open64-devel] Review request: fixing issues when building >>>> the compiler as 64-bit binaries >>>> >>>> I see. So this is similar to what Srf was doing, except she is dealing >>>> with file, line etc >>>> Sun >>>> >>>> On Fri, Aug 3, 2012 at 1:47 PM, Jian-Xin Lai <laij...@gmail.com> wrote: >>>> > The new ID is used for debug purpose in IPA phase. It tries to assign >>>> > each WN in different PU an unique ID. So that it's possible to track >>>> > the WN with the given ID across PUs. >>>> > >>>> > 2012/8/2 Sun Chan <sun.c...@gmail.com>: >>>> >> There is WHIRL version for incompatible WHIRL matching and >>>> >> identification. What is this new ID is about? >>>> >> Sun >>>> >> >>>> >> On Thu, Aug 2, 2012 at 5:05 PM, Jian-Xin Lai <laij...@gmail.com> >>>> wrote: >>>> >>> For the WHIRL ID for debug purpose, I think it's not very useful >>>> and >>>> >>> we can fully remove it. Otherwise the IR file generated by debug >>>> >>> compiler and opt compiler isn't compatible. >>>> >>> >>>> >>> 2012/7/29 Gilmore, Doug <doug.gilm...@amd.com>: >>>> >>>> I have put together a patch set that fixes various issues when >>>> >>>> building X86 targeting compiler as 64-bit binaries. >>>> >>>> >>>> >>>> With these changes the code generated by the 32-bit or 64-bit >>>> build of >>>> >>>> the compiler should be the same, whether the compiler is built for >>>> >>>> release or debug. >>>> >>>> >>>> >>>> Except for the register allocation stability issues, getting the >>>> >>>> 32-bit and 64-bit builds of the compiler to generate the same code >>>> for >>>> >>>> C and C++ programs required little change. However for Fortran >>>> >>>> a good deal of cleanup was needed in the FFE. >>>> >>>> >>>> >>>> I also added some configuration cleanup that made it easier to >>>> enable >>>> >>>> FFE debugging/tracing, and adding the debug ID field in a WHIRL >>>> Node >>>> >>>> that made it easier to track down the cause of different code >>>> being >>>> >>>> generated by the 32-bit and 64-bit builds of the compiler. >>>> >>>> >>>> >>>> Also included in the patch set is an update of the constant >>>> folding of >>>> >>>> intrinsics patch that I sent out a while back that just handled >>>> the >>>> >>>> real intrinsic. The updated version of this change (patch 5) >>>> includes >>>> >>>> changes made by Shivarama Rao which handles more intrinsics. I >>>> >>>> included this change in the patch set since by applying it first, >>>> the >>>> >>>> the patch associated with the commit to our internal source tree >>>> for >>>> >>>> the 64-bit cleanup applies much more cleanly to the main Open64 >>>> trunk. >>>> >>>> >>>> >>>> Sun had an issue with constant folding and FP rounding modes. I >>>> have >>>> >>>> looked into this and I believe that this is not a concern since >>>> the >>>> >>>> FP rounding mode interface routines to get and set the FP rounding >>>> >>>> mode was an SGI extension and AFAICS the implementation was never >>>> >>>> completed for Open64. >>>> >>>> >>>> >>>> To apply the patch set run: >>>> >>>> >>>> >>>> tar xf m64_trunk_v3.tar.bz2 >>>> >>>> for i in m64_trunk_v3/* ; do patch -p1 < $i ; done >>>> >>>> >>>> >>>> Here is how patches are broken up among various sub-systems: >>>> >>>> >>>> >>>> build subsystem: >>>> >>>> 0001-Add-configure-option-with-build-ffe-optimize.patch >>>> >>>> 0002-Add-configuration-option-enable-whirl-id.patch >>>> >>>> 0003-Configure-fix-to-handle-GNU-Gold-linker.patch >>>> >>>> 0012-Host-related-configure-settings-should-not-be-determ.patch >>>> >>>> 0013-Default-X8664-build-is-now-64-bit-when-building-on-a.patch >>>> >>>> >>>> >>>> Fortran FE: >>>> >>>> 0004-Allow-constant-folding-of-Intrinsic-in-parameter-sta.patch >>>> >>>> 0005-Allow-count-intrinsic-to-be-used-in-a-F90-specificat.patch >>>> >>>> 0006-Cleanup-64-bit-compilation-issues-in-FFE.patch >>>> >>>> >>>> >>>> CG: >>>> >>>> 0007-Fix-stability-issues-in-register-allocator.patch >>>> >>>> 0008-Fixed-bug-in-expand_strcmp_bb-exposed-by-64-bit-buil.patch >>>> >>>> 0010-Fixed-bug-in-Targ_Print-for-X8664-when-printing-C10-.patch >>>> >>>> 0011-Fixed-bug-999-C10-constant-emission-problem.patch >>>> >>>> >>>> >>>> WOPT: >>>> >>>> 0009-Fixed-typo-in-warning-string-for-error-message-type-.patch >>>> >>>> >>>> >>>> Note that patch 5 and 9 are trivial fixes to separate problems, >>>> >>>> That I probably should have sent out separate messages for their >>>> review. >>>> >>>> >>>> >>>> Also it would be good to test builds to check that nothing >>>> >>>> has been broken on other architectures. >>>> >>>> >>>> >>>> Could gatekeepers take a look at these changes (and others that >>>> can >>>> >>>> build on other architectures test them) when they have the chance? >>>> >>>> >>>> >>>> Thanks, >>>> >>>> >>>> >>>> Doug >>>> >>>> >>>> >>>> ------------------------------------------------------------------ >>>> ------------ >>>> >>>> Live Security Virtual Conference >>>> >>>> Exclusive live event will cover all the ways today's security and >>>> >>>> threat landscape has changed and how IT managers can respond. >>>> Discussions >>>> >>>> will include endpoint security, mobile security and the latest in >>>> malware >>>> >>>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ >>>> >>>> _______________________________________________ >>>> >>>> Open64-devel mailing list >>>> >>>> Open64-devel@lists.sourceforge.net >>>> >>>> https://lists.sourceforge.net/lists/listinfo/open64-devel >>>> >>>> >>>> >>> >>>> >>> >>>> >>> >>>> >>> -- >>>> >>> Regards, >>>> >>> Lai Jian-Xin >>>> >>> >>>> >>> ------------------------------------------------------------------- >>>> ----------- >>>> >>> Live Security Virtual Conference >>>> >>> Exclusive live event will cover all the ways today's security and >>>> >>> threat landscape has changed and how IT managers can respond. >>>> Discussions >>>> >>> will include endpoint security, mobile security and the latest in >>>> malware >>>> >>> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ >>>> >>> _______________________________________________ >>>> >>> Open64-devel mailing list >>>> >>> Open64-devel@lists.sourceforge.net >>>> >>> https://lists.sourceforge.net/lists/listinfo/open64-devel >>>> > >>>> > >>>> > >>>> > -- >>>> > Regards, >>>> > Lai Jian-Xin >>> >>> >> >> > > > > ------------------------------------------------------------------------------ > Live Security Virtual Conference > Exclusive live event will cover all the ways today's security and > threat landscape has changed and how IT managers can respond. Discussions > will include endpoint security, mobile security and the latest in malware > threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ > _______________________________________________ > Open64-devel mailing list > Open64-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/open64-devel ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ Open64-devel mailing list Open64-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/open64-devel
