| Message ID | 20131016231314.GA16300@ibm-tiger.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 16, 2013 at 4:13 PM, Michael Meissner <meissner@linux.vnet.ibm.com> wrote: > On Wed, Oct 16, 2013 at 02:34:56PM -0700, Sriraman Tallam wrote: >> On Tue, Oct 15, 2013 at 10:54 PM, Alan Modra <amodra@gmail.com> wrote: >> > On Tue, Oct 15, 2013 at 02:45:23PM -0700, Sriraman Tallam wrote: >> >> I committed this patch after making the above change. >> > >> > /src/gcc-virgin/gcc/config/rs6000/rs6000.c: At global scope: >> > /src/gcc-virgin/gcc/config/rs6000/rs6000.c:31122:29: error: invalid conversion from ‘void (*)(cl_target_option*)’ to ‘void (*)(cl_target_option*, gcc_options*)’ [-fpermissive] >> > /src/gcc-virgin/gcc/config/rs6000/rs6000.c:31122:29: error: invalid conversion from ‘void (*)(cl_target_option*)’ to ‘void (*)(gcc_options*, cl_target_option*)’ [-fpermissive] >> >> This patch fixes it, ok to submit? > > No. I have just committed a fix for this. Your patch does not replicate the > rs6000_isa_flags_explicit field to be a GCC option. Presumably the intent of > the 57756 patch was to remove references to the global variables. Your patch > still references those variables. What I did was to move the isa explicit flag > to be a target variable, so that it is preserved in the gcc_options structure > like everything else. > > However, I wonder why you committed the original changes with changes to the > powerpc backend, and DID NOT build a powerpc and fix the compilation errors. I was unable to build a native powerpc compiler. I checked for build_target_node and build_optimization_node throughout and changed rs6000 because it had references. I did not realize function_specific_save and function_specific_restore have to be changed. Sorry for breaking it. Sri > > 2013-10-16 Michael Meissner <meissner@linux.vnet.ibm.com> > > PR target/57756 > * config/rs6000/rs6000.opt (rs6000_isa_flags_explicit): Move the > explicit isa flag to be an options variable, instead of using > global_options_set. Remove define from rs6000.h. > * config/rs6000/rs6000.h (rs6000_isa_flags_explicit): Likewise. > > * config/rs6000/rs6000.c (rs6000_option_override_internal): > Initialize rs6000_isa_flags_explicit. > (rs6000_function_specific_save): Add gcc_options* parameter, so > that the powerpc builds after the 2013-10-15 changes. > (rs6000_function_specific_restore): Likewise. > > -- > Michael Meissner, IBM > IBM, M/S 2506R, 550 King Street, Littleton, MA 01460, USA > email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797
On Wed, Oct 16, 2013 at 04:23:56PM -0700, Sriraman Tallam wrote: > On Wed, Oct 16, 2013 at 4:13 PM, Michael Meissner > <meissner@linux.vnet.ibm.com> wrote: > > On Wed, Oct 16, 2013 at 02:34:56PM -0700, Sriraman Tallam wrote: > >> On Tue, Oct 15, 2013 at 10:54 PM, Alan Modra <amodra@gmail.com> wrote: > >> > On Tue, Oct 15, 2013 at 02:45:23PM -0700, Sriraman Tallam wrote: > >> >> I committed this patch after making the above change. > >> > > >> > /src/gcc-virgin/gcc/config/rs6000/rs6000.c: At global scope: > >> > /src/gcc-virgin/gcc/config/rs6000/rs6000.c:31122:29: error: invalid conversion from ‘void (*)(cl_target_option*)’ to ‘void (*)(cl_target_option*, gcc_options*)’ [-fpermissive] > >> > /src/gcc-virgin/gcc/config/rs6000/rs6000.c:31122:29: error: invalid conversion from ‘void (*)(cl_target_option*)’ to ‘void (*)(gcc_options*, cl_target_option*)’ [-fpermissive] > >> > >> This patch fixes it, ok to submit? > > > > No. I have just committed a fix for this. Your patch does not replicate the > > rs6000_isa_flags_explicit field to be a GCC option. Presumably the intent of > > the 57756 patch was to remove references to the global variables. Your patch > > still references those variables. What I did was to move the isa explicit flag > > to be a target variable, so that it is preserved in the gcc_options structure > > like everything else. > > > > However, I wonder why you committed the original changes with changes to the > > powerpc backend, and DID NOT build a powerpc and fix the compilation errors. > > I was unable to build a native powerpc compiler. I checked for > build_target_node and build_optimization_node throughout and changed > rs6000 because it had references. I did not realize > function_specific_save and function_specific_restore have to be > changed. Sorry for breaking it. The gcc110 machine in the compile farm can be used to build native powerpc64 toolchains. In addition, the problem would have shown up if you had built a cross compiler. You presumably missed the references in rs6000.h that defined rs6000_isa_flags_explicit as using the global_options_set structure.
On Wed, Oct 16, 2013 at 7:23 PM, Sriraman Tallam <tmsriram@google.com> wrote: > I was unable to build a native powerpc compiler. I checked for > build_target_node and build_optimization_node throughout and changed > rs6000 because it had references. I did not realize > function_specific_save and function_specific_restore have to be > changed. Sorry for breaking it. As Mike replied, gcc110 is available. Richard Biener's approval was dependent upon successful bootstrap and passing the regression testsuite, which you did not even attempt, nor did you try to build a cross-compiler. You also did not contact the rs6000 maintainer (me) nor the last person who changed the code in question (Mike). How is Google going to change its patch commit policies to ensure that this does not happen again? Thanks, David
On Wed, Oct 16, 2013 at 6:06 PM, David Edelsohn <dje.gcc@gmail.com> wrote: > On Wed, Oct 16, 2013 at 7:23 PM, Sriraman Tallam <tmsriram@google.com> wrote: > >> I was unable to build a native powerpc compiler. I checked for >> build_target_node and build_optimization_node throughout and changed >> rs6000 because it had references. I did not realize >> function_specific_save and function_specific_restore have to be >> changed. Sorry for breaking it. > > As Mike replied, gcc110 is available. Richard Biener's approval was > dependent upon successful bootstrap and passing the regression > testsuite, which you did not even attempt, nor did you try to build a > cross-compiler. You also did not contact the rs6000 maintainer (me) > nor the last person who changed the code in question (Mike). Yes, I should have done this. > How is Google going to change its patch commit policies to ensure that > this does not happen again? Looking at our best practices document, it points to http://gcc.gnu.org/contribute.html and mentions the compile farm. So, this is my fault alone for not testing powerpc & mips. Sri > > Thanks, David
On Wed, Oct 16, 2013 at 6:06 PM, David Edelsohn <dje.gcc@gmail.com> wrote: > On Wed, Oct 16, 2013 at 7:23 PM, Sriraman Tallam <tmsriram@google.com> wrote: > >> I was unable to build a native powerpc compiler. I checked for >> build_target_node and build_optimization_node throughout and changed >> rs6000 because it had references. I did not realize >> function_specific_save and function_specific_restore have to be >> changed. Sorry for breaking it. > > As Mike replied, gcc110 is available. Richard Biener's approval was > dependent upon successful bootstrap and passing the regression > testsuite, which you did not even attempt, nor did you try to build a > cross-compiler. This is an oversight. I agree that it is better to test on multiple platforms for large changes like this. In the past, Sri has been very attentive to any fallouts due to his changes, so is this time. >You also did not contact the rs6000 maintainer (me) > nor the last person who changed the code in question (Mike). You can count on us to send more patches your way for testing in the future :) > > How is Google going to change its patch commit policies to ensure that > this does not happen again? > I am not sure what you mean here. We only have one policy to follow for trunk GCC -- GCC's own policy. thanks, David > Thanks, David
On Wed, 2013-10-16 19:40:21 -0700, Xinliang David Li <davidxl@google.com> wrote: > On Wed, Oct 16, 2013 at 6:06 PM, David Edelsohn <dje.gcc@gmail.com> wrote: > > On Wed, Oct 16, 2013 at 7:23 PM, Sriraman Tallam <tmsriram@google.com> wrote: > > > I was unable to build a native powerpc compiler. I checked for > > > build_target_node and build_optimization_node throughout and > > > changed rs6000 because it had references. I did not realize > > > function_specific_save and function_specific_restore have to be > > > changed. Sorry for breaking it. > > > > As Mike replied, gcc110 is available. Richard Biener's approval > > was dependent upon successful bootstrap and passing the regression > > testsuite, which you did not even attempt, nor did you try to > > build a cross-compiler. > > This is an oversight. I agree that it is better to test on multiple > platforms for large changes like this. In the past, Sri has been > very attentive to any fallouts due to his changes, so is this time. As of speaking about multiple platforms... This patch didn't only produce fallout on rs6k, but also for quite a number of other architectures. I already send one message (it can be found in the archives at http://gcc.gnu.org/ml/gcc-patches/2013-10/msg01156.html) listing quite a number of targets that are broken right now. This situation didn't change significantly since they started to fail. Please have a look at my build robot[1]. All targets (except nds32, nios2 and arceb) used to build. Don't get me wrong. I don't want to overly blame Sriraman for breaking it in the first place. Shit happens. But please have an eye on fixing the fallout, timely. MfG, JBG [1] http://toolchain.lug-owl.de/buildbot/?limit=2000
On Wed, Oct 16, 2013 at 6:06 PM, David Edelsohn <dje.gcc@gmail.com> wrote: > How is Google going to change its patch commit policies to ensure that > this does not happen again? There is nothing to change. Google follows http://gcc.gnu.org/contribute.html, like everyone else. Sri just fixed the oversight and if there is any other fallout from his patch, he will address it. I don't see anything out of the ordinary here. Diego.
Index: gcc/config/rs6000/rs6000.opt =================================================================== --- gcc/config/rs6000/rs6000.opt (revision 203723) +++ gcc/config/rs6000/rs6000.opt (working copy) @@ -30,6 +30,9 @@ TargetSave HOST_WIDE_INT x_rs6000_isa_flags ;; Miscellaneous flag bits that were set explicitly by the user +Variable +HOST_WIDE_INT rs6000_isa_flags_explicit + TargetSave HOST_WIDE_INT x_rs6000_isa_flags_explicit Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 203723) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -2796,6 +2796,10 @@ rs6000_option_override_internal (bool gl = ((global_init_p || target_option_default_node == NULL) ? NULL : TREE_TARGET_OPTION (target_option_default_node)); + /* Remember the explicit arguments. */ + if (global_init_p) + rs6000_isa_flags_explicit = global_options_set.x_rs6000_isa_flags; + /* On 64-bit Darwin, power alignment is ABI-incompatible with some C library functions, so warn about it. The flag may be useful for performance studies from time to time though, so don't disable it @@ -29995,19 +29999,22 @@ rs6000_set_current_function (tree fndecl /* Save the current options */ static void -rs6000_function_specific_save (struct cl_target_option *ptr) +rs6000_function_specific_save (struct cl_target_option *ptr, + struct gcc_options *opts) { - ptr->x_rs6000_isa_flags = rs6000_isa_flags; - ptr->x_rs6000_isa_flags_explicit = rs6000_isa_flags_explicit; + ptr->x_rs6000_isa_flags = opts->x_rs6000_isa_flags; + ptr->x_rs6000_isa_flags_explicit = opts->x_rs6000_isa_flags_explicit; } /* Restore the current options */ static void -rs6000_function_specific_restore (struct cl_target_option *ptr) +rs6000_function_specific_restore (struct gcc_options *opts, + struct cl_target_option *ptr) + { - rs6000_isa_flags = ptr->x_rs6000_isa_flags; - rs6000_isa_flags_explicit = ptr->x_rs6000_isa_flags_explicit; + opts->x_rs6000_isa_flags = ptr->x_rs6000_isa_flags; + opts->x_rs6000_isa_flags_explicit = ptr->x_rs6000_isa_flags_explicit; (void) rs6000_option_override_internal (false); } Index: gcc/config/rs6000/rs6000.h =================================================================== --- gcc/config/rs6000/rs6000.h (revision 203723) +++ gcc/config/rs6000/rs6000.h (working copy) @@ -593,9 +593,6 @@ extern int rs6000_vector_align[]; #define MASK_PROTOTYPE OPTION_MASK_PROTOTYPE #endif -/* Explicit ISA options that were set. */ -#define rs6000_isa_flags_explicit global_options_set.x_rs6000_isa_flags - /* For power systems, we want to enable Altivec and VSX builtins even if the user did not use -maltivec or -mvsx to allow the builtins to be used inside of #pragma GCC target or the target attribute to change the code level for a