Re: [PATCH] rs6000: Add a builtin to read the time base register on PowerPC

["Tulio Magno Quites Machado Filho" <tuliom at linux dot vnet dot ibm dot com>]

Hi Segher,

Segher Boessenkool <segher@kernel.crashing.org> writes:
> > Add __builtin_ppc_get_timebase to read the time base register 
> > on PowerPC.
> > This is required for applications that measure time at high 
> > frequencies
> > with high precision that can't afford a syscall.
>
> For things that do mftb with high frequency, maybe you should 
> also add a
> builtin that does just an mftb, i.e. returns a 32-bit result on 
> 32-bit
> implementations.

Are you thinking in a function that returns only the TBL?
I don't think such a builtin would make sense on a 64-bit 
environment, right?
Do you have a suggestion for its name?

> Please add documentation for the new builtin(s).

Sure!

> > --- a/gcc/config/rs6000/rs6000-builtin.def
> > +++ b/gcc/config/rs6000/rs6000-builtin.def
> > @@ -1429,6 +1429,9 @@ BU_SPECIAL_X (RS6000_BUILTIN_RSQRT, 
> > "__builtin_rsqrt", RS6000_BTM_FRSQRTE,
> >  BU_SPECIAL_X (RS6000_BUILTIN_RSQRTF, "__builtin_rsqrtf", 
> >  RS6000_BTM_FRSQRTES,
> >  	      RS6000_BTC_FP)
> >
> > +BU_SPECIAL_X (RS6000_BUILTIN_GET_TB, 
> > "__builtin_ppc_get_timebase",
> > +	     RS6000_BTM_POWERPC, RS6000_BTC_MISC)
>
> RS6000_BTM_POWERPC does not exist anymore.  RS6000_BTM_ALWAYS?

I'm replacing.

> > +/* Expand an expression EXP that calls a builtin without 
> > arguments.  */
> > +static rtx
> > +rs6000_expand_noop_builtin (enum insn_code icode, rtx target)
>
> "noop" gives the wrong idea, "zeroop" perhaps?

zeroop is much better.

> > +(define_expand "get_timebase"
>
> You should probably prefix this with powerpc_ or rs6000_ as 
> well.
> The existing code is not very consistent in this.

OK.

> > +  [(use (match_operand:DI 0 "gpc_reg_operand" ""))]
> > +  ""
> > +  "
> > +{
> > +  if (TARGET_POWERPC64)
> > +    emit_insn (gen_get_timebase_ppc64 (operands[0]));
> > +  else if (TARGET_POWERPC)
> > +    emit_insn (gen_get_timebase_ppc32 (operands[0]));
> > +  else
> > +    FAIL;
> > +  DONE;
> > +}")
>
> TARGET_POWERPC is always true.

OK.

> > +(define_insn "get_timebase_ppc32"
> > +  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
> > +        (unspec_volatile:DI [(const_int 0)] UNSPECV_GETTB))
> > +   (clobber (match_scratch:SI 1 "=r"))]
> > +  "TARGET_POWERPC && !TARGET_POWERPC64"
> > +{
> > +    return "mftbu %0\;"
> > +	   "mftb %L0\;"
> > +	   "mftbu %1\;"
> > +	   "cmpw %0,%1\;"
> > +	   "bne- $-16";
> > +})
>
> This only works for WORDS_BIG_ENDIAN.

Yes.

> You should say you clobber CR0 here I think; actually, allow any 
> CRn
> instead.

Yes.

> Does mftb work on all supported assemblers?  The machine 
> instruction
> is phased out, but some assemblers translate it to mfspr.

According to the Power ISA 2.06 they should translate it to mfspr.

> > +(define_insn "get_timebase_ppc64"
> > +  [(set (match_operand:DI 0 "gpc_reg_operand" "=r")
> > +        (unspec_volatile:DI [(const_int 0)] UNSPECV_GETTB))]
> > +  "TARGET_POWERPC64"
> > +{
> > +    return "mfspr %0, 268";
> > +})
>
> POWER3 needs mftb.

Nice catch!

> > +int
> > +main(void)
> > +{
> > +  uint64_t t1, t2, t3;
> > +
> > +  t1 = __builtin_ppc_get_timebase ();
> > +  t2 = __builtin_ppc_get_timebase ();
> > +  t3 = __builtin_ppc_get_timebase ();
> > +
> > +  if (t1 != t2 && t1 != t3 && t2 != t3)
> > +    return 0;
> > +
> > +  return 1;
> > +}
>
> On some systems the timebase runs at a rather low frequency, say 
> 20MHz.
> This test will spuriously fail there.  Waste a million CPU 
> cycles before
> reading TB the second time?

Yes.

Thank you,

--
Tulio Magno