Re: [PATCH, rs6000, testsuite] Fix PR target/64579, __TM_end __builtin_tend failed to return transactional state

[Segher Boessenkool <segher at kernel dot crashing dot org>]

On Tue, Apr 21, 2015 at 03:56:18PM -0500, Peter Bergner wrote:
> On Fri, 2015-03-20 at 17:41 -0500, Peter Bergner wrote:
> > On Fri, 2015-03-20 at 15:52 -0500, Segher Boessenkool wrote:
> > > Maybe it would be nicer if the builtin-expansion code handled the copy
> > > from cc, instead of stacking on RTL expanders.
> > 
> > That would allow getting rid of the expanders completely, which
> > would be nice.  I'd have to somehow add some type of RS6000_BTC_*
> > flag onto the builtin though, so I can tell during builtin expansion
> > whether I need to emit the cr copy code or not.
> 
> Ok, the patch below implements your suggestion.

It looks good, thanks.  Some minor comments...

> This patch also fixes some issues I hit with the tabortdc[i] and
> htm_m[ft]spr_<mode> patterns when used with -m32 -mpowerpc64.

Running the testsuite, or did you actually try to _use_ -m32 -mpowerpc64?  :-)

> +(define_insn "tabortdc"
>    [(set (match_operand:CC 3 "cc_reg_operand" "=x")
>  	(unspec_volatile:CC [(match_operand 0 "u5bit_cint_operand" "n")
> -			     (match_operand:SI 1 "gpc_reg_operand" "r")
> -			     (match_operand:SI 2 "gpc_reg_operand" "r")]
> +			     (match_operand:DI 1 "gpc_reg_operand" "r")
> +			     (match_operand:DI 2 "gpc_reg_operand" "r")]
>  			    UNSPECV_HTM_TABORTDC))]
> -  "TARGET_HTM"
> +  "TARGET_POWERPC64 && TARGET_HTM"
>    "tabortdc. %0,%1,%2"
>    [(set_attr "type" "htm")
>     (set_attr "length" "4")])

Maybe you can fold tabortdc with tabortwc now?  Use one UNSPEC name
for both, :GPR and <wd>?

> +	  case HTM_BUILTIN_TTEST: /* Alias for: tabortwci. 0,r0,0  */
> +	    op[nopnds++] = GEN_INT (0);
> +	    op[nopnds++] = gen_rtx_REG (SImode, 0);
> +	    op[nopnds++] = GEN_INT (0);

Is that really r0, isn't that (0|rA)?  [Too lazy to read the docs myself
right now, sorry.]

> +	if (attr & RS6000_BTC_CR)
> +	  {
> +	    if (fcode == HTM_BUILTIN_TBEGIN)
> +	      {
> +		/* Emit code to set TARGET to true or false depending on
> +		   whether the tbegin. instruction successfully or failed
> +		   to start a transaction.  We do this by placing the 1's
> +		   complement of CR's EQ bit into TARGET.  */
> +		rtx scratch = gen_reg_rtx (SImode);
> +		emit_insn (gen_rtx_SET (VOIDmode, scratch,
> +					gen_rtx_EQ (SImode, cr,
> +						     const0_rtx)));
> +		emit_insn (gen_rtx_SET (VOIDmode, target,
> +					gen_rtx_XOR (SImode, scratch,
> +						     GEN_INT (1))));
> +	      }
> +	    else
> +	      {
> +		/* Emit code to copy the 4-bit condition register field
> +		   CR into the least significant end of register TARGET.  */
> +		rtx scratch1 = gen_reg_rtx (SImode);
> +		rtx scratch2 = gen_reg_rtx (SImode);
> +		rtx subreg = simplify_gen_subreg (CCmode, scratch1, SImode, 0);
> +		emit_insn (gen_movcc (subreg, cr));
> +		emit_insn (gen_lshrsi3 (scratch2, scratch1, GEN_INT (28)));
> +		emit_insn (gen_andsi3 (target, scratch2, GEN_INT (0xf)));
> +	      }
> +	  }

Don't we have helper functions/expanders to do these moves?  Yuck.

> -/* { dg-final { scan-assembler-times "tabortdc\\." 1 } } */
> -/* { dg-final { scan-assembler-times "tabortdci\\." 1 } } */
> +/* { dg-final { scan-assembler-times "tabortdc\\." 1 { target lp64 } } } */
> +/* { dg-final { scan-assembler-times "tabortdci\\." 1 { target lp64 } } } */

This skips this test on -m32 -mpowerpc64, is that on purpose?


Segher