Re: [PATCH], Remove power9 fusion support, version 2
Hi Mike, Thanks for the changes. On Thu, Nov 08, 2018 at 04:28:52PM -0500, Michael Meissner wrote: > * config/rs6000/constraints.md (wF constraint): Update constraint > documentation for power8 fusion only. "so it is for ...", maybe? It is hard to understand your sentence. > +/* Do not allow SF/DFmode in GPR fusion. While the loads do occur, they > + are not common. */ Trailing whitespace. (twice) Okay for trunk with those trivialities fixed. Thanks! Segher
Re: [PATCH], Remove power9 fusion support
On Wed, Nov 07, 2018 at 01:36:50PM -0500, Michael Meissner wrote: > On Mon, Nov 05, 2018 at 04:09:23PM -0600, Segher Boessenkool wrote: > > Hi Mike, > > > > On Fri, Nov 02, 2018 at 02:37:34PM -0400, Michael Meissner wrote: > > > This patch removes all of the so-called power9 fusion support for the GCC > > > compiler. It leaves -mpower9-fusion as a deprecated switch in case > > > somebody > > > used it (the switch was never documented). > > > > As Mike Stump says, please just remove it. The option was never documented, > > most likely zero people use it, and those that do shouldn't have and can > > easily adjust. > > > > > [gcc] > > > 2018-11-02 Michael Meissner > > > > > > * config/rs6000/constraints.md (wF constraint): Only document the > > > wF constraint for power8 fusion. Remove documentation for power9 > > > fusion. > > > > It wasn't documented as being anything for p8 before. So that was wrong? > > The switch wasn't documented. In the constraint (which is what I'm changing > here), the constraint mentioned p9 fusion in the documentation string. Yeah; so it was wrong. k. > > > (rs6000_option_override_internal): Delete power9 fusion option > > > support. If we do -mcpu=power8 -mtune=power9, turn off power8 > > > fusion. > > > > That doesn't sound right. Either the -mcpu= or the -mtune= should turn > > it on, but neither should turn it off. It sounds like you want -mtune > > to say whether fusion is enabled or not? That sounds fine, but this > > should be implemented more directly (or more generically). > > Ok, I will look at it. Thanks. > > >/* Power8 currently will only do the fusion if the top 11 bits of the > > > addis > > > - value are all 1's or 0's. Ignore this restriction if we are testing > > > - advanced fusion. */ > > > - if (TARGET_P9_FUSION) > > > -return 1; > > > - > > > + value are all 1's or 0's. */ > > >return (IN_RANGE (value >> 16, -32, 31)); > > > }) > > > > I think this is top 12 bits equal, not 11, so [-16..15]. > > It is 11 bits, check section 12.1.12 in the power8 book IV. > > addis(SI) first 11 bits must be all 0’s or all 1’s As we discussed offline, it is 12. Segher
Re: [PATCH], Remove power9 fusion support
On Mon, Nov 05, 2018 at 04:09:23PM -0600, Segher Boessenkool wrote: > Hi Mike, > > On Fri, Nov 02, 2018 at 02:37:34PM -0400, Michael Meissner wrote: > > This patch removes all of the so-called power9 fusion support for the GCC > > compiler. It leaves -mpower9-fusion as a deprecated switch in case somebody > > used it (the switch was never documented). > > As Mike Stump says, please just remove it. The option was never documented, > most likely zero people use it, and those that do shouldn't have and can > easily adjust. > > > [gcc] > > 2018-11-02 Michael Meissner > > > > * config/rs6000/constraints.md (wF constraint): Only document the > > wF constraint for power8 fusion. Remove documentation for power9 > > fusion. > > It wasn't documented as being anything for p8 before. So that was wrong? The switch wasn't documented. In the constraint (which is what I'm changing here), the constraint mentioned p9 fusion in the documentation string. > > (rs6000_option_override_internal): Delete power9 fusion option > > support. If we do -mcpu=power8 -mtune=power9, turn off power8 > > fusion. > > That doesn't sound right. Either the -mcpu= or the -mtune= should turn > it on, but neither should turn it off. It sounds like you want -mtune > to say whether fusion is enabled or not? That sounds fine, but this > should be implemented more directly (or more generically). Ok, I will look at it. > > mpower9-fusion > > -Target Undocumented Report Mask(P9_FUSION) Var(rs6000_isa_flags) > > -Fuse certain operations together for better performance on power9. > > +Target Undocumented Mask(P9_FUSION) Var(rs6000_isa_flags) Deprecated > > Yeah just delete this please. Ok. > > @@ -1692,11 +1650,7 @@ (define_predicate "fusion_gpr_addis" > > return 0; > > > >/* Power8 currently will only do the fusion if the top 11 bits of the > > addis > > - value are all 1's or 0's. Ignore this restriction if we are testing > > - advanced fusion. */ > > - if (TARGET_P9_FUSION) > > -return 1; > > - > > + value are all 1's or 0's. */ > >return (IN_RANGE (value >> 16, -32, 31)); > > }) > > I think this is top 12 bits equal, not 11, so [-16..15]. It is 11 bits, check section 12.1.12 in the power8 book IV. addis(SI) first 11 bits must be all 0’s or all 1’s > > @@ -1762,14 +1718,13 @@ (define_predicate "fusion_gpr_mem_load" > > ;; Match a GPR load (lbz, lhz, lwz, ld) that uses a combined address in the > > ;; memory field with both the addis and the memory offset. Sign extension > > ;; is not handled here, since lha and lwa are not fused. > > -;; With P9 fusion, also match a fpr/vector load and float_extend > > (define_predicate "fusion_addis_mem_combo_load" > >(match_code "mem,zero_extend,float_extend") > > So float_extend should be deleted here? Yes. -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meiss...@linux.ibm.com, phone: +1 (978) 899-4797
Re: [PATCH], Remove power9 fusion support
Hi Mike, On Fri, Nov 02, 2018 at 02:37:34PM -0400, Michael Meissner wrote: > This patch removes all of the so-called power9 fusion support for the GCC > compiler. It leaves -mpower9-fusion as a deprecated switch in case somebody > used it (the switch was never documented). As Mike Stump says, please just remove it. The option was never documented, most likely zero people use it, and those that do shouldn't have and can easily adjust. > [gcc] > 2018-11-02 Michael Meissner > > * config/rs6000/constraints.md (wF constraint): Only document the > wF constraint for power8 fusion. Remove documentation for power9 > fusion. It wasn't documented as being anything for p8 before. So that was wrong? > * config/rs6000/predicates.md (p9_fusion_reg_operand): Delete, the > predicate only used for power9 fusion support. Just "Delete." > (fusion_gpr_addis): Drop support to allow fusing offsets where the > top 11 bits weren't all 0 or all 1's. > (fusion_gpr_mem_load): Add comment about not allowing SFmode or > DFmode in power8 fusion. > (fusion_addis_mem_combo_load): Drop power9 fusion support. Only > support power8 fusion. Add comment about not allowing SFmode or > DFmode in power8 fusion. > (fusion_offsettable_mem_operand): Delete, the predicate only used > for power9 fusion support. Just "Delete." The changelog should not explain *why* you are changing something. Put that in the commit message. The changelog is much easier to read without this. > * config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_NO_FUSION): New > option masks that is all of the ISA 2.07 bits except for fusion. > (ISA_2_7_MASKS_SERVER): Add fusion bits back in. > (ISA_3_0_MASKS_SERVER): Delete power9 fusion. Use ISA 2.07 bits > without enabling power8 fusion. > (POWERPC_MASKS): Delete power9 fusion option mask. > * config/rs6000/rs6000-protos.h (emit_fusion_load_store): Delete > function declarations used for power9 fusion. > (fusion_p9_p): Likewise. > (expand_fusion_p9_load): Likewise. > (expand_fusion_p9_store): Likewise. > (emit_fusion_p9_load): Likewise. > (emit_fusion_p9_store): Likewise. All of these are just "Delete." (Or "Delete declaration.") > * config/rs6000/rs6000.c (rs6000_debug_reg_global): Delete power9 > fusion debug print. "Delete." Well you get the idea... There are more of them later in here. > (rs6000_option_override_internal): Delete power9 fusion option > support. If we do -mcpu=power8 -mtune=power9, turn off power8 > fusion. That doesn't sound right. Either the -mcpu= or the -mtune= should turn it on, but neither should turn it off. It sounds like you want -mtune to say whether fusion is enabled or not? That sounds fine, but this should be implemented more directly (or more generically). > (rs6000_opt_masks): Delete power9 fusion option. > (emit_fusion_load): Rename function from emit_fusion_load_store. > Make the function static. Delete support for fusing stores, since > we are deleting power9 fusion. > (fusion_p9_p): Delete power9 fusion support functions. > (expand_fusion_p9_load): Likewise. > (expand_fusion_p9_store): Likewise. > (emit_fusion_p9_load): Likewise. > (emit_fusion_p9_store): Likewise. > * config/rs6000/rs6000.h: Delete comment about power9 fusion. > * config/rs6000/rs6000.md (UNSPEC_FUSION_P9): Delete, no longer > used since power9 fusion support has been deleted. > (GPR_FUSION iterator): Likewise. > (FPR_FUSION iterator): Likewise. > (power9 fusion peephole2's): Likewise. > (fusion_gpr___load): Likewise. > (fusion_gpr___store): Likewise. > (fusion_vsx___load): Likewise. > (fusion_vsx___store): Likewise. > (fusion_p9__constant): Likewise. > * config/rs6000/rs6000.opt (-mpower9-fusion): Mark as deprecated. > * doc/md.texi (PowerPC constraints): Update wF documentation. > > [gcc/testsuite] > 2018-11-02 Michael Meissner > > * gcc.target/powerpc/fusion3.c: Delete power9 fusion. > * gcc.target/powerpc/fusion4.c: Likewise. > --- gcc/config/rs6000/rs6000.opt > (.../svn+ssh://meiss...@gcc.gnu.org/svn/gcc/trunk/gcc/config/rs6000) > (revision 265537) > +++ gcc/config/rs6000/rs6000.opt (.../gcc/config/rs6000) (working copy) > @@ -499,8 +499,7 @@ Target Undocumented Var(rs6000_optimize_ > Analyze and remove doubleword swaps from VSX computations. > > mpower9-fusion > -Target Undocumented Report Mask(P9_FUSION) Var(rs6000_isa_flags) > -Fuse certain operations together for better performance on power9. > +Target Undocumented Mask(P9_FUSION) Var(rs6000_isa_flags) Deprecated Yeah just delete this please. > @@ -1692,11 +1650,7 @@ (define_predicate "fusion_gpr_addis" > return 0; > >/* Power8 currently will only do the fusion if the t
Re: [PATCH], Remove power9 fusion support
On Nov 2, 2018, at 11:37 AM, Michael Meissner wrote: > > As I discussed in my 2018 Cauldron talk, the PowerPC GCC compiler supported a > subset of the original design for fusion in the power9 hardware using > peepholes > to fuse together ADDIS instructions and floating point load/store operations. > > However, while fusion was part of the original power9 design, by the time the > machine came out, the fusion support was no longer part of the architecture. > > This patch removes all of the so-called power9 fusion support for the GCC > compiler. It leaves -mpower9-fusion as a deprecated switch So, I'd just remove the flag support as well. Anyone that hits on it, will want to examine their code and have the opportunity to fix it.
