Hi Segher, Thanks for the review comments!
on 2022/12/15 06:29, Segher Boessenkool wrote: > On Wed, Nov 30, 2022 at 04:30:13PM +0800, Kewen.Lin wrote: >> As PR104024 shows, the option -mpower10-fusion isn't guarded by >> -mcpu=power10, it causes compiler to fuse for some patterns >> even without power10 support and then causes ICE unexpectedly, >> this patch is to simply unmask it without power10 support, not >> emit any warnings as this option is undocumented. > > Yes, it mostly exists for debugging purposes (and also for testcase). > >> Besides, for some define_insns in fusion.md which use constraint >> v, it requires the condition VECTOR_UNIT_ALTIVEC_OR_VSX_P >> (<MODE>mode), otherwise it can cause ICE in reload, see test >> case pr104024-2.c. > > Please don't two separate things in one patch. It makes bisecting > harder than necessary, and perhaps more interesting to you: it makes > writing good changelog entries and commit messages harder. OK, will do. > >> --- a/gcc/config/rs6000/genfusion.pl >> +++ b/gcc/config/rs6000/genfusion.pl >> @@ -167,7 +167,7 @@ sub gen_logical_addsubf >> $inner_comp, $inner_inv, $inner_rtl, $inner_op, $both_commute, $c4, >> $bc, $inner_arg0, $inner_arg1, $inner_exp, $outer_arg2, $outer_exp, >> $ftype, $insn, $is_subf, $is_rsubf, $outer_32, $outer_42,$outer_name, >> - $fuse_type); >> + $fuse_type, $constraint_cond); >> KIND: foreach $kind ('scalar','vector') { >> @outer_ops = @logicals; >> if ( $kind eq 'vector' ) { >> @@ -176,12 +176,14 @@ sub gen_logical_addsubf >> $pred = "altivec_register_operand"; >> $constraint = "v"; >> $fuse_type = "fused_vector"; >> + $constraint_cond = "VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode) && "; >> } else { >> $vchr = ""; >> $mode = "GPR"; >> $pred = "gpc_reg_operand"; >> $constraint = "r"; >> $fuse_type = "fused_arith_logical"; >> + $constraint_cond = ""; >> push (@outer_ops, @addsub); >> push (@outer_ops, ( "rsubf" )); >> } > > I don't like this at all. Please use the "isa" attribute where needed? > Or do you need more in some cases? But, again, separate patch. This is to add one more condition for those define_insns, for example: @@ -1875,7 +1875,7 @@ (define_insn "*fuse_vand_vand" (match_operand:VM 1 "altivec_register_operand" "%v,v,v,v")) (match_operand:VM 2 "altivec_register_operand" "v,v,v,v"))) (clobber (match_scratch:VM 4 "=X,X,X,&v"))] - "(TARGET_P10_FUSION)" + "(VECTOR_UNIT_ALTIVEC_OR_VSX_P (<MODE>mode) && TARGET_P10_FUSION)" "@ vand %3,%1,%0\;vand %3,%3,%2 vand %3,%1,%0\;vand %3,%3,%2 It's to avoid the pseudo whose mode isn't available for register constraint v causes ICE during reload. I'm not sure how the "isa" attribute helps here, could you elaborate it? > >> + if (TARGET_POWER10 >> + && (rs6000_isa_flags_explicit & OPTION_MASK_P10_FUSION) == 0) >> + rs6000_isa_flags |= OPTION_MASK_P10_FUSION; >> + else if (!TARGET_POWER10 && TARGET_P10_FUSION) >> + rs6000_isa_flags &= ~OPTION_MASK_P10_FUSION; > > That's not right. If you want something like this you should check for > TARGET_POWER10 whenever you check for TARGET_P10_FUSION; but there > really is no reason at all to disable P10 fusion on other CPUs (neither > newer nor older!). Good point, and I just noticed that we should check tune setting instead of TARGET_POWER10 here? Something like: if (!(rs6000_isa_flags_explicit & OPTION_MASK_P10_FUSION)) { if (processor_target_table[tune_index].processor == PROCESSOR_POWER10) rs6000_isa_flags |= OPTION_MASK_P10_FUSION; else rs6000_isa_flags &= ~OPTION_MASK_P10_FUSION; } > >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/powerpc/pr104024-1.c >> @@ -0,0 +1,16 @@ >> +/* { dg-require-effective-target int128 } */ >> +/* { dg-options "-O1 -mdejagnu-cpu=power6 -mpower10-fusion" } */ > > Does this need -O1? If not, use -O2 please; if so, document it. > No, it doesn't, will use -O2 instead. BR, Kewen