[PATCH] [PowerPC] Add Hardware Transaction Memory builtins support

[Peter Bergner]

On Fri, 2015-02-20 at 11:22 -0200, Adhemerval Zanella wrote:
> This patch adds Hardware Transaction Memory (HTM) support supported by
> ISA 2.07 (POWER8).  The intrinsic support is based on GCC one [1], but
> currently only the 'PowerPC HTM Low Level Built-in Function' are
> implemented.

First off, it's good to see this being implemented.  Thanks!

Secondly, I will be submitting a "fix" fairly soon to GCC to modify
the low level GCC HTM builtins, so don't trust the GCC documentation
on the builtin return values. :-)  Basically, all of the builtins
(except for tbegin and the builtins for accessing the SPRs) will now
return the full 4-bit CR value as its return value (similar to the
current ttest builtin).  The tbegin builtin will continue to return
0 or 1, so we can have code like:

  if (__builtin_tbegin (0))
    ...transaction successfully started
  else
    ...transaction failed to start

...using an efficient branch off of the EQ bit.



I recommend adding the XL defined intrinsics at some point, since they
are easily implemented as always inline functions in a header file
(htmxlintrin.h) using the low level builtins and will allow common code
across XL, GCC and CLANG on POWER and eventually S390, once/if they add
HTM support to LLVM.





> +def int_ppc_tbegin :
> +      Intrinsic<[llvm_i32_ty], [llvm_i32_ty], []>;
> +def int_ppc_tend :
> +      Intrinsic<[llvm_i32_ty], [llvm_i32_ty], []>;

Don't you need the "GCCBuiltin<"__builtin_t*" like the ones below?



> +def int_ppc_tcheck : GCCBuiltin<"__builtin_tcheck">,
> +      Intrinsic<[llvm_i32_ty], [llvm_i32_ty], []>;

Warning, as part of my GCC changes, I will be removing the argument to
the __builtin_tcheck() builtin.  The insn operand is to describe which
CR field the insn will write to and it doesn't make sense for the
user to decide that, so I removed it and allow the compiler to decide
which CR field to use.



> +def int_ppc_tsr :
> +      Intrinsic<[llvm_i32_ty], [llvm_i32_ty], []>;

Ditto regarding use of "GCCBuiltin<"__builtin_t*"?




> diff --git a/lib/Target/PowerPC/PPCISelLowering.cpp b/lib/Target/PowerPC/PPCISelLowering.cpp
> index 1258d96..e9300e7 100644
> --- a/lib/Target/PowerPC/PPCISelLowering.cpp
> +++ b/lib/Target/PowerPC/PPCISelLowering.cpp
> @@ -7848,7 +7848,7 @@ PPCTargetLowering::EmitInstrWithCustomInserter(MachineInstr *MI,
>      BuildMI(*BB, MI, dl, TII->get(TargetOpcode::COPY),
>              MI->getOperand(0).getReg())
>        .addReg(isEQ ? PPC::CR0EQ : PPC::CR0GT);
> -  } else {
> +  } else  {
>      llvm_unreachable("Unexpected instr type to insert");
>    }

Some unintended white space addition?




> diff --git a/lib/Target/PowerPC/PPCInstrHTM.td b/lib/Target/PowerPC/PPCInstrHTM.td
[snip]
> +//===----------------------------------------------------------------------===//
> +//
> +// This file describes the Hardware Transactional Memory extension to the i
> +// PowerPC instruction set.

Stray "i" -> s/to the i/to the/




> +// Builtins
> +
> +// All HTM instructions, with exception os tcheck, sets bit EQ in CR0 as 0 in case of
> +// a success.  So the XORI pattern is 'flip' the bit to return 1 as a success.

This is only true of the tbegin. instruction, which is one of the
reasons
why the current GCC code doesn't make sense for the other builtins and
why I'm going to change them.  Maybe this would read better as:

// All HTM instructions, with the exception of tcheck, set CR0 with the
// value of the MSR Transaction State (TS) bits that exist before the
// instruction is executed.  For tbegin., the EQ bit in CR0 can be used
// to determine whether the transaction was successfully started (0) or
// failed (1).  We use an XORI pattern to 'flip' the bit to match the
// tbegin builtin API which defines a return value of 1 as success.






> +def : Pat<(int_ppc_tcheck i32:$BF),
> +          (TCHECK (HTM_get_imm imm:$BF))>;

Again, we're going to remove the argument for tcheck, so this will
need to change.


Peter