This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Fwd: [PATCH] Scheduling result adjustment to enable macro-fusion


Thanks for the comments. One question inlined. Preparing another patch
addressing the comments.

Regards,
Wei Mi.

On Tue, Oct 15, 2013 at 1:35 PM, Jeff Law <law@redhat.com> wrote:
> On 10/03/13 12:24, Wei Mi wrote:
>>
>> Thanks,
>> Wei Mi.
>>
>> 2013-10-03  Wei Mi  <wmi@google.com>
>>
>>          * gcc/config/i386/i386.c (memory_address_length): Extract a part
>>          of code to rip_relative_addr_p.
>>          (rip_relative_addr_p): New Function.
>>          (ix86_macro_fusion_p): Ditto.
>>          (ix86_macro_fusion_pair_p): Ditto.
>>          * gcc/config/i386/i386.h: Add new tune features about
>> macro-fusion.
>>          * gcc/config/i386/x86-tune.def (DEF_TUNE): Ditto.
>>          * gcc/doc/tm.texi: Generated.
>>          * gcc/doc/tm.texi.in: Ditto.
>>          * gcc/haifa-sched.c (try_group_insn): New Function.
>>          (group_insns_for_macro_fusion): Ditto.
>>          (sched_init): Call group_insns_for_macro_fusion.
>>          * gcc/sched-rgn.c (add_branch_dependences): Keep insns in
>>          a SCHED_GROUP at the end of BB to remain their location.
>>          * gcc/target.def: Add two hooks: macro_fusion_p and
>>          macro_fusion_pair_p.
>
> I'm not going to comment on the x86 specific stuff -- I'll defer to the port
> maintainers for that.
>
>
>
>> index 61eaaef..d6726a9 100644
>> --- a/gcc/haifa-sched.c
>> +++ b/gcc/haifa-sched.c
>> @@ -6519,6 +6519,44 @@ setup_sched_dump (void)
>>                  ? stderr : dump_file);
>>   }
>>
>> +static void
>> +try_group_insn (rtx insn)
>
> You need a comment for this function.
>

Ok, will add comment for it.

>
>
>> +{
>> +  unsigned int condreg1, condreg2;
>> +  rtx cc_reg_1;
>> +  rtx prev;
>> +
>> +  targetm.fixed_condition_code_regs (&condreg1, &condreg2);
>> +  cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
>> +  prev = prev_nonnote_nondebug_insn (insn);
>> +  if (!any_condjump_p (insn)
>> +      || !reg_referenced_p (cc_reg_1, PATTERN (insn))
>> +      || !prev
>> +      || !modified_in_p (cc_reg_1, prev))
>> +    return;
>
> I'd test !any_condjump_p at the start of this function before calling the
> target hook.  If insn isn't a conditional jump, then all the other work is
> totally useless.

Ok. will fix it.

>
> Aren't you just trying to see if we have a comparison feeding the
> conditional jump and if they're already adjacent?  Do you actually need to
> get the condition code regs to do that test?
>

Yes, I am trying to see if we have a comparison feeding the
conditional jump and if they're already adjacent. Do you have more
easier way to do that test?

>
>> +
>> +  /* Different microarchitectures support macro fusions for different
>> +     combinations of insn pairs.  */
>> +  if (!targetm.sched.macro_fusion_pair_p
>> +      || !targetm.sched.macro_fusion_pair_p (prev, insn))
>> +    return;
>> +
>> +  SCHED_GROUP_P (insn) = 1;
>
> I'm surprised that SCHED_GROUP_P worked -- I've tried to do similar stuff in
> the past and ran into numerous problems trying to hijack SCHED_GROUP_P for
> this kind of purpose.
>
>
>
>>
>>   static void haifa_init_only_bb (basic_block, basic_block);
>> diff --git a/gcc/sched-rgn.c b/gcc/sched-rgn.c
>> index e1a2dce..156359e 100644
>> --- a/gcc/sched-rgn.c
>> +++ b/gcc/sched-rgn.c
>> @@ -2443,6 +2443,8 @@ add_branch_dependences (rtx head, rtx tail)
>>        cc0 setters remain at the end because they can't be moved away from
>>        their cc0 user.
>>
>> +     Predecessors of SCHED_GROUP_P instructions at the end remain at the
>> end.
>> +
>>        COND_EXEC insns cannot be moved past a branch (see e.g. PR17808).
>>
>>        Insns setting TARGET_CLASS_LIKELY_SPILLED_P registers (usually
>> return
>> @@ -2465,7 +2467,8 @@ add_branch_dependences (rtx head, rtx tail)
>>   #endif
>>                   || (!reload_completed
>>                       && sets_likely_spilled (PATTERN (insn)))))
>> -        || NOTE_P (insn))
>> +        || NOTE_P (insn)
>> +        || (last != 0 && SCHED_GROUP_P (last)))
>>       {
>>         if (!NOTE_P (insn))
>>          {
>
> This looks like a straighforward bugfix and probably should go forward
> independent of this enhancement.

Ok, I will separate it into another patch.

>
> Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]