Re: [PATCH]: R10000 Needs LL/SC Workaround in Gcc

[Richard Sandiford <rdsandiford at googlemail dot com>]

Kumba <kumba@gentoo.org> writes:
> Richard Sandiford wrote:
>> 
>> To be clear, the first option above was to check -- in mips_override_options --
>> that -mfix-r10000 is only used in cases where -mbranch-likely is in effect.
>> If we pick that option, it would be an error to use -mfix-r10000 in
>> other cases, and any code protected by TARGET_FIX_R10000 would be free
>> to use branch-likely instructions.  (Actually, we should use sorry()
>> instead of error() to report something like this.)
>
> [snip]
>
>> That's the second option above, yes.  In other words, -mfix-r10000
>> would support both -mbranch-likely and -mno-branch-likely, and act
>> accordingly.
>
> So do I need to worry about modifying the asm templates at all?  Or is
> that only needed if pursuing option #2?

You need to modify the asm templates whatever you do.

> The branch-likely stuff is only going to work for MIPS-II or higher
> targets.  In the odd (but possible) cases where MIPS-I might be used
> with -mfix-r10000, I assume we'll still have to emit 28 nops prior to
> a beq/beqz instruction.  Is this already taken care of someplace?

Hmm?  No, option #2 was supposed to include this work.

I feel we're talking at cross-purposes, so just to be clear:

 - In current gcc sources, the code generated by the __sync*() functions
   is not suitable for R10K processors.  That's true for all current
   command-line options.  We therefore need to add a new command-line
   option (-mfix-r10000) that selects the required behaviour.

 - As you said in your original message, there are two workarounds
   for the R10K errata:

   a) Make the asm templates use branch-likely instructions instead of
      normal branches when -mfix-r10000 is in effect.

   b) Separate loops by at least 28 instructions when -mfix-r10000 is
      in effect.

   Both workarounds require work, because gcc does neither of these
   things at present.  However, (a) is much easier to implement than (b).

   So it seems to me that there are two possible ways of implementing
   the -mfix-r10000 option:

   1) Only do (a).  Make it an error to use -mfix-r10000 when
      branch-likely instructions are not available.  We should
      check for this error in mips_override_options and use
      sorry() rather than error() to report it.

      Branch-likely instructions are not available if either:

      i) the command line includes -mno-branch-likely; or
      ii) both of the following hold:
          - the selected architecture does not provide branch-likely
            instructions; and
          - the command line does not include -mbranch-likely.

      The C condition for this is the one I gave in my previous message:

      (target_flags_explicit & MASK_BRANCHLIKELY) == 0
       ? !ISA_HAS_BRANCH_LIKELY
       ? !TARGET_BRANCH_LIKELY)

      So we should give up and call sorry() if:

          TARGET_FIX_R10000
          && (target_flags_explicit & MASK_BRANCHLIKELY) == 0
              ? !ISA_HAS_BRANCHLIKELY
              ? !TARGET_BRANCHLIKELY)

      is true.

      If we take this approach, any gcc code guarded by TARGET_FIX_R10000
      can make free use of branch-likely instructions.  No separate
      *_BRANCHLIKELY condition would be needed.

      In other words, the asm template could always use branch-likely
      instructions when TARGET_FIX_R10000 is true, and always use the
      current branch sequences otherwise.

   2) Implement both (a) and (b).  In this case, any gcc code guarded
      by TARGET_FIX_R10000 would need to check whether branch-likely
      instructions are available.  If they are, we can use either
      workaround (a) or workaroudn (b).  If they aren't, we must
      use workaround (b).

   These two options correspond to the two in my original reply.

>> ...that's a good question.  My take is "no".  I don't think we want
>> -mfix-r10000 to enable branch-likely instructions in cases where
>> it isn't necessary for R10000 errata.  If we take the first option,
>> we can simply raise an error if:
>
> Hmm, okay.  Might this work to enable -mbranch-likely if -mfix-r10000?
> (Kind of guessing by looking at other segments of code).
>
>    if ((target_flags_explicit & MASK_BRANCHLIKELY) == 0)
>      {
>        if (ISA_HAS_BRANCHLIKELY
>            && (optimize_size
>                || (!(target_flags_explicit & MASK_FIX_R10000) == 0)
>                || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0))
>          target_flags |= MASK_BRANCHLIKELY;
>        else
>          target_flags &= ~MASK_BRANCHLIKELY;
>      }

No.  What I was trying to say in the quoted text was: -mfix-r10000
should have _no_ effect on whether branch-likely instructions are
available for general use.

As the comment above this code says:

  /* If neither -mbranch-likely nor -mno-branch-likely was given
     on the command line, set MASK_BRANCHLIKELY based on the target
     architecture and tuning flags.  Annulled delay slots are a
     size win, so we only consider the processor-specific tuning
     for !optimize_size.  */

In other words, this bit of the condition:

           (optimize_size
            || (mips_tune_info->tune_flags & PTF_AVOID_BRANCHLIKELY) == 0)

is a _tuning_ decision.  It is used in cases where both
TARGET_BRANCHLIKELY and !TARGET_BRANCH_LIKELY would be OK from
a correctness standpoint.

So suppose we only implement workaround (a) (== option (1) above).
We now need branch-likely instructions _for correctness_, but only
in certain asm templates.  It's therefore OK to override the _tuning_
decision for those asm templates: correctness trumps tuning.  But we
shouldn't change the tuning decision for _other_ branches (i.e. for
branches where -mfix-r10000 does not require a branch-likely instruction).

> My understanding so far for -mfix-r10000:
> - Gets enabled if -march=r10000 is passed (done)

Yes, provided that you never override an explicit -mfix-r10000 or
-mno-fix-r10000.

>> Yeah, I was wondering that too.  I did a search, but couldn't
>> find anything.
>
> It seems we just need to use nop only and not worry about ssnop.

Actually, I meant: I was wondering about the fact that there seems
to be no online copy of the errata sheet that describes this problem.
I've only ever seen a description of the workaround.  I've never seen
a verbatim copy of the errata itself.

Richard