Re: [Open64-devel] FW: FW: Review request: fixing issues when building the compiler as 64-bit binar
Brought to you by:
ributzka,
suneeljain
Menu
▾
▴
Re: [Open64-devel] FW: FW: Review request: fixing issues when building the compiler as 64-bit binaries
|
From: Gilmore, D. <Dou...@am...> - 2012-08-19 21:47:32
|
> -----Original Message----- > From: Sun Chan [mailto:sun...@gm...] > Sent: Monday, August 13, 2012 3:09 PM > To: Gilmore, Doug > Cc: open64-devel > Subject: Re: [Open64-devel] FW: FW: Review request: fixing issues when > building the compiler as 64-bit binaries > > 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) This is to get rid of a compiler warning, in osprey/common/com/*/config_targ.h where Is_Target_64bit and Is_Target_32bit are also defined, for example the X8664 version is: #define Is_Target_64bit() (Target_ABI == ABI_n64) #define Is_Target_32bit() (Target_ABI == ABI_n32) I added a comment about this issue. > > // 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)]) I need more clarification of what the issue is? Yes I could have changed all of the blindly changed to variable types and casts from long to int, but I thought it was better to use typedefs to describe the data being manipulated. > > > // Their opening comment - yet they fail to grok that it doesn't help > the situation long term - see below It does make the situation better long term, since it reduces the amount of conditionally compiled code that needs to be maintained. There are bugs and features lacking in the 64-bit build of the FFE: https://bugs.open64.net/show_bug.cgi?id=467 All of these issues have been resolve by the typing change. Fixes and enhancements that were conditionally compiled for HOST32/_TARGET32 now work when the FFE is compiled 32 or 64 bit. (per the commit log message these macros were renamed to _HOST32_OR_64 and _TARGET32_OR_64). The motivating factor for these changes was how to get the Open64 to generate the same code for when the compiler is compiled 32 or 64 bit. Except for a few issues, we were close to doing so for C and C++. For Fortran we were not. I don't see how we could address this problem without making these changes. > > 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 I was thinking about that when I started, however this is coding style that is not at all present in the FFE. There are no references to int32_t and the only use of INT32 are in the function prototypes to the WHIRL generation routines in i_cvrt.h. I did go ahead and make the change to the typedefs for the const_pool_type, name_pool_word_type*, and wds_type to be int32_t. Note that these types are only to provide alignment of the objects being stored in the constant and name tables (or the type being used to move data around when tables are being resized). > // 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)) Sorry this wasn't a good way to address the problem I was fixing. The code in the FFE (function pre_parse_format in p_io.c) needs to deal with the possibility that there is a mis-match in the "word block size" of the used by the library code and the "word block size" used for storing constants in the FFE. I cleaned up the code so that format.h included in the FFE. Also I noticed that I overlooked some compiler warnings that were being generated, due to a missing fix to bug 918. I attached a patch for the fix, which I needed to put first in the patch set to reproduce the problem as reported in the bug report. I attached tarball with the new set of patches. Doug > > On Tue, Aug 14, 2012 at 5:22 AM, Gilmore, Doug <Dou...@am...> > 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:dco...@gm...] > > 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 <Dou...@am...> > wrote: > >> Hi David, > >> > >> You already reviewed this change right? > >> > >> Doug > >> > >> -----Original Message----- > >> From: Sun Chan [mailto:sun...@gm...] > >> 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 > <Dou...@am...> 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...@gm...] > >>>> 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 <la...@gm...> > 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...@gm...>: > >>>> >> 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 > <la...@gm...> > >>>> 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 <Dou...@am...>: > >>>> >>>> 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 > >>>> >>>> > >>>> >>>> ------------------------------------------------------------- > >>>> >>>> _______________________________________________ > >>>> >>>> Open64-devel mailing list > >>>> >>>> Ope...@li... > >>>> >>>> https://lists.sourceforge.net/lists/listinfo/open64-devel > >>>> >>>> > >>>> >>> > >>>> >>> > >>>> >>> > >>>> >>> -- > >>>> >>> Regards, > >>>> >>> Lai Jian-Xin > >>>> >>> > >>>> >>> -------------------------------------------------------------- > ----- > >>>> >>> Open64-devel mailing list > >>>> >>> Ope...@li... > >>>> >>> https://lists.sourceforge.net/lists/listinfo/open64-devel > >>>> > > >>>> > > >>>> > > >>>> > -- > >>>> > Regards, > >>>> > Lai Jian-Xin > >>> > >>> > >> > >> > > > > > > |