Re: [PATCH] S/390: Hardware transactional memory support
On 14/08/13 17:33, Richard Henderson wrote: > On 08/14/2013 05:44 AM, Torvald Riegel wrote: >> Can we instead move the successful path of the HTM fastpath to the >> _ITM_beginTransaction asm function, and just do the fallback and retry >> code in beginend.cc? > > Certainly, but this seems like a larger re-design, and I was thinking of > a shorter-term solution. > Before http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00820.html no FPR appears to be used as GPR save slot. So it should be safe for 4.8 branch. Skimming through the EC12 compiled libitm.so disassembly all FPR uses appear to be limited to the ITM_* accessor functions. I think 4.8 should be ok as is and we perhaps could go with the patches from Andi for mainline? Bye, -Andreas-
Re: [PATCH] S/390: Hardware transactional memory support
On 02/08/13 23:05, Richard Henderson wrote:
> On 08/02/2013 04:45 AM, Andreas Krebbel wrote:
>> ! XCFLAGS="${XCFLAGS} -mzarch -mhtm -msoft-float"
>
> Not good, since _ITM_R{F,D,E,CF,CD,CE} should return values in
> floating point registers; similarly for the write accessors.
Right. I've reverted that change on mainline and 4.8 branch.
Bye,
-Andreas-
Re: [PATCH] S/390: Hardware transactional memory support
On 08/14/2013 05:44 AM, Torvald Riegel wrote: > Can we instead move the successful path of the HTM fastpath to the > _ITM_beginTransaction asm function, and just do the fallback and retry > code in beginend.cc? Certainly, but this seems like a larger re-design, and I was thinking of a shorter-term solution. r~
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, 2013-08-13 at 12:09 -0700, Richard Henderson wrote:
> On 08/13/2013 11:26 AM, Jakub Jelinek wrote:
> > On Fri, Aug 02, 2013 at 11:05:33AM -1000, Richard Henderson wrote:
> >> On 08/02/2013 04:45 AM, Andreas Krebbel wrote:
> >>> ! XCFLAGS="${XCFLAGS} -mzarch -mhtm -msoft-float"
> >>
> >> Not good, since _ITM_R{F,D,E,CF,CD,CE} should return values in
> >> floating point registers; similarly for the write accessors.
> >
> > So, would it be enough to compile just beginend.cc with -msoft-float
> > and the rest normally?
>
> No.
>
> >From what I understand of the s390 restriction, we can't touch the
> floating point registers after starting a transaction, at least until
> we're committed to restoring them all.
>
> Which means that we have to have everything along the path from
> htm_begin_success == false until a longjmp restores them. This path
> includes a call to std::operator new, so that makes it a non-starter.
>
> Better, I think, to pass the gtm_jmpbuf to htm_begin. Then we can do
>
> uint32_t ret = __builtin_tbegin_nofloat (NULL);
> if (!htm_begin_success(ret))
> // restore fpu state from jb
> return ret;
>
> at which point we're back to normal and we can do whatever we want
> within the normal abi wrt the fpu.
Can we instead move the successful path of the HTM fastpath to the
_ITM_beginTransaction asm function, and just do the fallback and retry
code in beginend.cc? So, add a tbegin (or xbegin) and the check of
serial_lock to the asm function as in Andi's patch
(http://gcc.gnu.org/ml/gcc-patches/2013-01/msg00640.html), but keep the
restart policy code (ie, the serial lock read/write locking for the
wait, etc.) in beginend.cc? The latter could return a new code that
tells the asm function to try the HTM fastpath again; we might also want
to use a separate bit in the arguments to gtm_thread::begin_transaction
to tell it whether the HTM fastpath failed before.
This way, we get lower overhead when the HTM fastpath succeeds right
away because we can do the minimally necessary thing in asm; we still
can change the retry policies easily; and we can restore the floating
point registers to a sane state for any libitm C/C++ code that we might
call without having to do any restore inside of the C/C++ code.
Thoughts? I'll put working on a draft of this for x86 on my list.
Re: [PATCH] S/390: Hardware transactional memory support
On 08/13/2013 11:26 AM, Jakub Jelinek wrote:
> On Fri, Aug 02, 2013 at 11:05:33AM -1000, Richard Henderson wrote:
>> On 08/02/2013 04:45 AM, Andreas Krebbel wrote:
>>> ! XCFLAGS="${XCFLAGS} -mzarch -mhtm -msoft-float"
>>
>> Not good, since _ITM_R{F,D,E,CF,CD,CE} should return values in
>> floating point registers; similarly for the write accessors.
>
> So, would it be enough to compile just beginend.cc with -msoft-float
> and the rest normally?
No.
>From what I understand of the s390 restriction, we can't touch the
floating point registers after starting a transaction, at least until
we're committed to restoring them all.
Which means that we have to have everything along the path from
htm_begin_success == false until a longjmp restores them. This path
includes a call to std::operator new, so that makes it a non-starter.
Better, I think, to pass the gtm_jmpbuf to htm_begin. Then we can do
uint32_t ret = __builtin_tbegin_nofloat (NULL);
if (!htm_begin_success(ret))
// restore fpu state from jb
return ret;
at which point we're back to normal and we can do whatever we want
within the normal abi wrt the fpu.
r~
Re: [PATCH] S/390: Hardware transactional memory support
On Fri, Aug 02, 2013 at 11:05:33AM -1000, Richard Henderson wrote:
> On 08/02/2013 04:45 AM, Andreas Krebbel wrote:
> > ! XCFLAGS="${XCFLAGS} -mzarch -mhtm -msoft-float"
>
> Not good, since _ITM_R{F,D,E,CF,CD,CE} should return values in
> floating point registers; similarly for the write accessors.
So, would it be enough to compile just beginend.cc with -msoft-float
and the rest normally?
Or what routines in particular must be prevented from using float registers
implicitly?
Jakub
Re: [PATCH] S/390: Hardware transactional memory support
On 08/02/2013 04:45 AM, Andreas Krebbel wrote:
> ! XCFLAGS="${XCFLAGS} -mzarch -mhtm -msoft-float"
Not good, since _ITM_R{F,D,E,CF,CD,CE} should return values in
floating point registers; similarly for the write accessors.
r~
Re: [PATCH] S/390: Hardware transactional memory support
On Fri, 2013-08-02 at 15:16 +0200, Andreas Krebbel wrote:
> On 02/08/13 13:31, Torvald Riegel wrote:
> > On Fri, 2013-06-21 at 12:23 +0200, Andreas Krebbel wrote:
> >> Index: libitm/config/s390/target.h
> >> ===
> >> *** libitm/config/s390/target.h.orig
> >> --- libitm/config/s390/target.h
> >> ***
> >
> > [...]
> >
> >> + static inline uint32_t
> >> + htm_begin ()
> >> + {
> >> + return __builtin_tbegin_nofloat (NULL);
> >> + }
> >
> > I'm not quite sure I understand why the nofloat variant is sufficient;
> > is this because we don't use FPRs in the abort/restart code of libitm
> > (and the FPRs are known to be clobbered after starting a txn)?
>
> Yes. To my understanding there are 2 potential issues when the FPRs are not
> saved.
>
> 1. The abort code will read corrupted FPR content for FPRs:
> - which are live across tbegin and
> - are updated in the TX body and
> - are read in the abort code
> The abort code will see the updated value from the TX body.
>
> Since libitm implements TX begins as function calls only call-saved registers
> can be live across a
> tbegin. But all the call-saved FPRs are saved in _ITM_beginTransaction and
> get restored when doing
> the longjmp back into the user code. So this should be no problem.
>
> 2. The other problem is that the ABI might get violated when aborting into a
> callee which already
> returned and there are FPRs which have been modified afterwards. In that case
> the compiler would not
> have generated FPR save/restore instructions in the function
> prologue/epilogue of the callee. But in
> fact there are modified FPRs when exiting the second time.
>
> But again this should be no problem thanks to the setjmp/longjmp semantics of
> _ITM_beginTransaction
> and GTM_longjmp. If the TX body modified a call-saved FPR it will get
> restored on the abort path
> back from libitm to the user code.
>
>
> As long as libitm does not use FPRs itself this should be safe without having
> tbegin clobbering FPRs.
And I suppose that even if it would use FPRs, it shouldn't ever access
memory locations touched in user code.
Thanks for the explanation. Some of this was "obvious", sorry, but
that's what happens when a mental context switch is incomplete :) Next
time, I'd prefer a summary of such reasoning to be added as a comment in
the source; it's just too easy to forget about all the subtleties...
> > There is a bit in the properties passed to _ITM_beginTransaction which
> > states whether the txn has floating point update code
> > (pr_hasNoFloatUpdate). Would this be useful?
>
> This probably would be useful in the ITM_beginTransaction / GTM_longjmp
> implementations which in
> that case could try to avoid the FPR save/restores. But Richard mentioned
> that these bits so far are
> never set by GCC since it is lacking the required infos from the register
> allocator.
Another thing that comes to mind is that it might be useful to put parts
of the HTM fast path into the asm pieces of _ITM_beginTransaction. That
way, one can use exactly the amount of SW setjmp one needs given what's
easy to save/restore for the TM; this might decrease the startup costs
of a txn somewhat. Andi Kleen posted a patch for how to do this for RTM
a while ago (but it had some issues):
http://gcc.gnu.org/ml/gcc-patches/2013-01/msg01189.html
Perhaps something along these lines (while avoiding the issues :) )
would be useful for s/390 and Power too.
Torvald
Re: [PATCH] S/390: Hardware transactional memory support
On 02/08/13 16:36, Peter Bergner wrote:
> On Fri, 2013-08-02 at 16:26 +0200, Andreas Krebbel wrote:
>> On 02/08/13 16:23, Peter Bergner wrote:
>> As long as libitm does not use FPRs itself this should be safe without
>> having tbegin clobbering FPRs.
>
> Is it a given that s390 doesn't use FPRs without explicit use of
> floating point types? I ask, because on POWER, we can and do
> generate floating point code without explicit use of double,
> float, etc. Maybe s390 is safe in that respect.
S/390 used to be on the safe side regarding this but with mainline we started
using FPRs as GPR save
slots. As Richard suggested we should build libitm with -msoft-float in order
to prevent this within
libitm. Unfortunately I forgot about this in my S/390 libitm patch. So I'll
commit the following:
Index: libitm/configure.tgt
===
*** libitm/configure.tgt.orig 2013-07-30 07:42:29.0 +
--- libitm/configure.tgt2013-08-02 14:43:18.641206151 +
*** case "${target_cpu}" in
*** 109,115
ARCH=x86
;;
s390|s390x)
! XCFLAGS="${XCFLAGS} -mzarch -mhtm"
ARCH=s390
;;
--- 109,115
ARCH=x86
;;
s390|s390x)
! XCFLAGS="${XCFLAGS} -mzarch -mhtm -msoft-float"
ARCH=s390
;;
Re: [PATCH] S/390: Hardware transactional memory support
On Fri, 2013-08-02 at 16:26 +0200, Andreas Krebbel wrote: > On 02/08/13 16:23, Peter Bergner wrote: > > On Fri, 2013-08-02 at 15:16 +0200, Andreas Krebbel wrote: > >> Since libitm implements TX begins as function calls only call-saved > >> registers can be live across a > >> tbegin. But all the call-saved FPRs are saved in _ITM_beginTransaction and > >> get restored when doing > >> the longjmp back into the user code. So this should be no problem. > > > > Except that the htm_begin() routines are declared static inline functions, > > so when they're inlined, you aren't protected by the semantics of a call > > anymore, are you? > > They get inlined into libitm code but not into the user code. As I understand > it in the user code > will always be a call to _ITM_beginTransaction. Sure, that protects the user code, but what about the libitm code? >From your previous comment: > As long as libitm does not use FPRs itself this should be safe without > having tbegin clobbering FPRs. Is it a given that s390 doesn't use FPRs without explicit use of floating point types? I ask, because on POWER, we can and do generate floating point code without explicit use of double, float, etc. Maybe s390 is safe in that respect. Peter
Re: [PATCH] S/390: Hardware transactional memory support
On 02/08/13 16:23, Peter Bergner wrote: > On Fri, 2013-08-02 at 15:16 +0200, Andreas Krebbel wrote: >> Since libitm implements TX begins as function calls only call-saved >> registers can be live across a >> tbegin. But all the call-saved FPRs are saved in _ITM_beginTransaction and >> get restored when doing >> the longjmp back into the user code. So this should be no problem. > > Except that the htm_begin() routines are declared static inline functions, > so when they're inlined, you aren't protected by the semantics of a call > anymore, are you? They get inlined into libitm code but not into the user code. As I understand it in the user code will always be a call to _ITM_beginTransaction. -Andreas- > > Peter > > >
Re: [PATCH] S/390: Hardware transactional memory support
On Fri, 2013-08-02 at 15:16 +0200, Andreas Krebbel wrote: > Since libitm implements TX begins as function calls only call-saved registers > can be live across a > tbegin. But all the call-saved FPRs are saved in _ITM_beginTransaction and > get restored when doing > the longjmp back into the user code. So this should be no problem. Except that the htm_begin() routines are declared static inline functions, so when they're inlined, you aren't protected by the semantics of a call anymore, are you? Peter
Re: [PATCH] S/390: Hardware transactional memory support
On 02/08/13 13:31, Torvald Riegel wrote:
> On Fri, 2013-06-21 at 12:23 +0200, Andreas Krebbel wrote:
>> Index: libitm/config/s390/target.h
>> ===
>> *** libitm/config/s390/target.h.orig
>> --- libitm/config/s390/target.h
>> ***
>
> [...]
>
>> + static inline uint32_t
>> + htm_begin ()
>> + {
>> + return __builtin_tbegin_nofloat (NULL);
>> + }
>
> I'm not quite sure I understand why the nofloat variant is sufficient;
> is this because we don't use FPRs in the abort/restart code of libitm
> (and the FPRs are known to be clobbered after starting a txn)?
Yes. To my understanding there are 2 potential issues when the FPRs are not
saved.
1. The abort code will read corrupted FPR content for FPRs:
- which are live across tbegin and
- are updated in the TX body and
- are read in the abort code
The abort code will see the updated value from the TX body.
Since libitm implements TX begins as function calls only call-saved registers
can be live across a
tbegin. But all the call-saved FPRs are saved in _ITM_beginTransaction and get
restored when doing
the longjmp back into the user code. So this should be no problem.
2. The other problem is that the ABI might get violated when aborting into a
callee which already
returned and there are FPRs which have been modified afterwards. In that case
the compiler would not
have generated FPR save/restore instructions in the function prologue/epilogue
of the callee. But in
fact there are modified FPRs when exiting the second time.
But again this should be no problem thanks to the setjmp/longjmp semantics of
_ITM_beginTransaction
and GTM_longjmp. If the TX body modified a call-saved FPR it will get restored
on the abort path
back from libitm to the user code.
As long as libitm does not use FPRs itself this should be safe without having
tbegin clobbering FPRs.
> There is a bit in the properties passed to _ITM_beginTransaction which
> states whether the txn has floating point update code
> (pr_hasNoFloatUpdate). Would this be useful?
This probably would be useful in the ITM_beginTransaction / GTM_longjmp
implementations which in
that case could try to avoid the FPR save/restores. But Richard mentioned that
these bits so far are
never set by GCC since it is lacking the required infos from the register
allocator.
> (BTW, sorry for the late response. I don't read gcc-patches frequently,
> so please CC me on libitm-related things.)
No problem. I'll try to remember this and cc you next time.
Bye,
-Andreas-
Re: [PATCH] S/390: Hardware transactional memory support
On Fri, 2013-06-21 at 12:23 +0200, Andreas Krebbel wrote:
> Index: libitm/config/s390/target.h
> ===
> *** libitm/config/s390/target.h.orig
> --- libitm/config/s390/target.h
> ***
[...]
> + static inline uint32_t
> + htm_begin ()
> + {
> + return __builtin_tbegin_nofloat (NULL);
> + }
I'm not quite sure I understand why the nofloat variant is sufficient;
is this because we don't use FPRs in the abort/restart code of libitm
(and the FPRs are known to be clobbered after starting a txn)?
There is a bit in the properties passed to _ITM_beginTransaction which
states whether the txn has floating point update code
(pr_hasNoFloatUpdate). Would this be useful?
(BTW, sorry for the late response. I don't read gcc-patches frequently,
so please CC me on libitm-related things.)
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, Jul 16, 2013 at 07:39:28PM +0200, Jakub Jelinek wrote:
> Looks good to me from quick skimming.
Ok. I've applied the following patch after successful testing.
Bye,
-Andreas-
2013-07-17 Andreas Krebbel
* config/s390/s390.c: (s390_expand_builtin): Allow -mhtm to be
enabled without -march=zEC12.
* config/s390/s390.h (TARGET_HTM): Do not require EC12 machine
flags to be set.
2013-07-17 Andreas Krebbel
* acinclude.m4: Add htm asm check for s390.
* configure.tgt: Add -mhtm and -Wa,-march=zEC12 to the options.
* configure: Regenerate.
* config/s390/target.h: Remove __HTM__ check.
(htm_available): Call getauxval to get hwcaps and check whether
HTM is available or not.
---
gcc/config/s390/s390.c |7 !!!
gcc/config/s390/s390.h |3 !!!
libitm/acinclude.m4 | 11 +++
libitm/config/s390/target.h | 23 !!!
libitm/configure| 36
libitm/configure.tgt|2 +-
6 files changed, 48 insertions(+), 1 deletion(-), 33 modifications(!)
Index: libitm/acinclude.m4
===
*** libitm/acinclude.m4.orig
--- libitm/acinclude.m4
*** powerpc*)
*** 135,140
--- 135,151
AC_DEFINE(HAVE_AS_HTM, 1, [Define to 1 if the assembler supports HTM.])
fi
;;
+ s390*)
+ AC_CACHE_CHECK([if the assembler supports HTM], libitm_cv_as_htm, [
+ save_CFLAGS="$CFLAGS"
+ CFLAGS="$CFLAGS -march=zEC12"
+ AC_TRY_COMPILE([], [asm("tbegin 0,0; tend");],
+ [libitm_cv_as_htm=yes], [libitm_cv_as_htm=no])
+ CFLAGS="$save_CFLAGS"])
+ if test x$libitm_cv_as_htm = xyes; then
+ AC_DEFINE(HAVE_AS_HTM, 1, [Define to 1 if the assembler supports HTM.])
+ fi
+ ;;
esac])
sinclude(../libtool.m4)
Index: libitm/config/s390/target.h
===
*** libitm/config/s390/target.h.orig
--- libitm/config/s390/target.h
***
*** 22,32
see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, Jul 16, 2013 at 07:03:18PM +0200, Andreas Krebbel wrote: > On Tue, Jul 16, 2013 at 07:24:31AM +0200, Jakub Jelinek wrote: > > And on s/390, right now we enable HTM support in libitm when configured for > > -march=zEC12 by default (which isn't ideal). > > Ok. What about the following patch (untested so far)? > > It basically copies what the Power folks are doing with getauxval. > I've also added the configure check for HTM assembler support which so > far was missing on S/390. Looks good to me from quick skimming. Jakub
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, Jul 16, 2013 at 07:24:31AM +0200, Jakub Jelinek wrote:
> And on s/390, right now we enable HTM support in libitm when configured for
> -march=zEC12 by default (which isn't ideal).
Ok. What about the following patch (untested so far)?
It basically copies what the Power folks are doing with getauxval.
I've also added the configure check for HTM assembler support which so
far was missing on S/390.
Bye,
-Andreas-
---
gcc/config/s390/s390.c |7 !!!
gcc/config/s390/s390.h |3 !!!
libitm/acinclude.m4 | 11 +++
libitm/config/s390/target.h | 23 !!!
libitm/configure| 36
libitm/configure.tgt|2 +-
6 files changed, 48 insertions(+), 1 deletion(-), 33 modifications(!)
Index: libitm/acinclude.m4
===
*** libitm/acinclude.m4.orig
--- libitm/acinclude.m4
*** powerpc*)
*** 135,140
--- 135,151
AC_DEFINE(HAVE_AS_HTM, 1, [Define to 1 if the assembler supports HTM.])
fi
;;
+ s390*)
+ AC_CACHE_CHECK([if the assembler supports HTM], libitm_cv_as_htm, [
+ save_CFLAGS="$CFLAGS"
+ CFLAGS="$CFLAGS -march=zEC12"
+ AC_TRY_COMPILE([], [asm("tbegin 0,0; tend");],
+ [libitm_cv_as_htm=yes], [libitm_cv_as_htm=no])
+ CFLAGS="$save_CFLAGS"])
+ if test x$libitm_cv_as_htm = xyes; then
+ AC_DEFINE(HAVE_AS_HTM, 1, [Define to 1 if the assembler supports HTM.])
+ fi
+ ;;
esac])
sinclude(../libtool.m4)
Index: libitm/config/s390/target.h
===
*** libitm/config/s390/target.h.orig
--- libitm/config/s390/target.h
***
*** 22,32
see the files COPYING3 and COPYING.RUNTIME respectively. If not, see
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, 2013-07-16 at 09:49 -0500, Peter Bergner wrote: > On Tue, 2013-07-16 at 08:08 -0400, David Edelsohn wrote: > > Then I agree that the hunk should be reverted so that HTM does not imply > > POWER8. > > Ok, I'll bootstrap and regtest just to be sure there is no unintended > fallout for such a small path before committing. Thanks. Ok, committed as revision 200985. Thanks. Peter
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, 2013-07-16 at 08:08 -0400, David Edelsohn wrote: > Then I agree that the hunk should be reverted so that HTM does not imply > POWER8. Ok, I'll bootstrap and regtest just to be sure there is no unintended fallout for such a small path before committing. Thanks. Peter
Re: [PATCH] S/390: Hardware transactional memory support
On Tue, Jul 16, 2013 at 1:24 AM, Jakub Jelinek wrote: > On Mon, Jul 15, 2013 at 10:40:04PM -0500, Peter Bergner wrote: >> On Mon, 2013-07-15 at 23:03 -0400, David Edelsohn wrote: >> > On Mon, Jul 15, 2013 at 4:26 PM, Peter Bergner >> > wrote: >> > > David, do you prefer reverting the above hunk from the Power HTM >> > > patch or should I add the associated -mno-* options to the building >> > > of libitm? >> > >> > How is libitm built for Intel? The principle of least surprises >> > suggests following that precedent. >> >> They use -mrtm (like we use -mhtm) to build libitm, but it looks like >> their -mrtm does not enable any other isa flags like we currently are >> doing with -mhtm. Meaning their -mrtm option is independent of any >> -mcpu values while ours is not. If we revert the patch I mentioned, >> then I think we will match what Intel is doing. >> Hopefully Jakub will correct me if I am wrong. > > Yes, that is my understanding of it too. On Intel we have: > #define OPTION_MASK_ISA_RTM_SET OPTION_MASK_ISA_RTM > #define OPTION_MASK_ISA_RTM_UNSET OPTION_MASK_ISA_RTM > which means that -mrtm doesn't set any other options except for itself > and -mno-rtm doesn't reset other options. libitm is built with -mrtm, > assuming that the only thing the -mrtm switch affects are the HTM builtins > and that those will only be found explicitly in code guarded with the > htm_available () runtime check. > Right now, -mhtm on PowerPC basically implies -march=power8 if I understand > the code well, and libitm is built with it, which means essentially that > when gcc is configured for a pre-power8 CPU, libitm will work just fine on > power8 (including HTM support), but when running on power7 and earlier > it might very well SIGILL, because the implicit -march=power8 could affect > even code not guarded by htm_available (). > And on s/390, right now we enable HTM support in libitm when configured for > -march=zEC12 by default (which isn't ideal). Then I agree that the hunk should be reverted so that HTM does not imply POWER8. Thanks, David
Re: [PATCH] S/390: Hardware transactional memory support
On Mon, Jul 15, 2013 at 10:40:04PM -0500, Peter Bergner wrote: > On Mon, 2013-07-15 at 23:03 -0400, David Edelsohn wrote: > > On Mon, Jul 15, 2013 at 4:26 PM, Peter Bergner wrote: > > > David, do you prefer reverting the above hunk from the Power HTM > > > patch or should I add the associated -mno-* options to the building > > > of libitm? > > > > How is libitm built for Intel? The principle of least surprises > > suggests following that precedent. > > They use -mrtm (like we use -mhtm) to build libitm, but it looks like > their -mrtm does not enable any other isa flags like we currently are > doing with -mhtm. Meaning their -mrtm option is independent of any > -mcpu values while ours is not. If we revert the patch I mentioned, > then I think we will match what Intel is doing. > Hopefully Jakub will correct me if I am wrong. Yes, that is my understanding of it too. On Intel we have: #define OPTION_MASK_ISA_RTM_SET OPTION_MASK_ISA_RTM #define OPTION_MASK_ISA_RTM_UNSET OPTION_MASK_ISA_RTM which means that -mrtm doesn't set any other options except for itself and -mno-rtm doesn't reset other options. libitm is built with -mrtm, assuming that the only thing the -mrtm switch affects are the HTM builtins and that those will only be found explicitly in code guarded with the htm_available () runtime check. Right now, -mhtm on PowerPC basically implies -march=power8 if I understand the code well, and libitm is built with it, which means essentially that when gcc is configured for a pre-power8 CPU, libitm will work just fine on power8 (including HTM support), but when running on power7 and earlier it might very well SIGILL, because the implicit -march=power8 could affect even code not guarded by htm_available (). And on s/390, right now we enable HTM support in libitm when configured for -march=zEC12 by default (which isn't ideal). Jakub
Re: [PATCH] S/390: Hardware transactional memory support
On Mon, 2013-07-15 at 23:03 -0400, David Edelsohn wrote: > On Mon, Jul 15, 2013 at 4:26 PM, Peter Bergner wrote: > > David, do you prefer reverting the above hunk from the Power HTM > > patch or should I add the associated -mno-* options to the building > > of libitm? > > How is libitm built for Intel? The principle of least surprises > suggests following that precedent. They use -mrtm (like we use -mhtm) to build libitm, but it looks like their -mrtm does not enable any other isa flags like we currently are doing with -mhtm. Meaning their -mrtm option is independent of any -mcpu values while ours is not. If we revert the patch I mentioned, then I think we will match what Intel is doing. Hopefully Jakub will correct me if I am wrong. Peter
Re: [PATCH] S/390: Hardware transactional memory support
On Mon, Jul 15, 2013 at 4:26 PM, Peter Bergner wrote: > On Mon, 2013-07-15 at 21:32 +0200, Jakub Jelinek wrote: >> Have you considered trying it to work even when libitm itself isn't built >> for zEC12 or later only? I mean, both the i?86/x86_64 and powerpc* libitm >> HTM don't define htm_available as unconditional true, they check in some way >> whether the CPU supports HTM and return true if yes, and as needed some >> parts or whole of libitm is compiled with some compiler option that >> allows the HTM instructions to be generated. >> >> I see your patch will currently error out if you have HTM builtins >> insode of code, -mtx and don't enable -march=zEC12, could that be changed, >> so that -mtx is essentially independent on the chosen CPU? > > Now that you mention this, my Power HTM patch enabled other ISA flags too, > if you used -mhtm. Like so: > >/* For the newer switches (vsx, dfp, etc.) set some of the older options, > unless the user explicitly used the -mno- to disable the code. > */ > - if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO) > + if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO || TARGET_HTM) > rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~rs6000_isa_flags_explicit); > > I think I agree with you that we shouldn't do that, since adding in > those extra flags with -mhtm basically means libitm can only really > run on a power8 system instead of the default -mcpu system. > > David, do you prefer reverting the above hunk from the Power HTM > patch or should I add the associated -mno-* options to the building > of libitm? How is libitm built for Intel? The principle of least surprises suggests following that precedent. - David
Re: [PATCH] S/390: Hardware transactional memory support
On Mon, 2013-07-15 at 21:32 +0200, Jakub Jelinek wrote: > Have you considered trying it to work even when libitm itself isn't built > for zEC12 or later only? I mean, both the i?86/x86_64 and powerpc* libitm > HTM don't define htm_available as unconditional true, they check in some way > whether the CPU supports HTM and return true if yes, and as needed some > parts or whole of libitm is compiled with some compiler option that > allows the HTM instructions to be generated. > > I see your patch will currently error out if you have HTM builtins > insode of code, -mtx and don't enable -march=zEC12, could that be changed, > so that -mtx is essentially independent on the chosen CPU? Now that you mention this, my Power HTM patch enabled other ISA flags too, if you used -mhtm. Like so: /* For the newer switches (vsx, dfp, etc.) set some of the older options, unless the user explicitly used the -mno- to disable the code. */ - if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO) + if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO || TARGET_HTM) rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~rs6000_isa_flags_explicit); I think I agree with you that we shouldn't do that, since adding in those extra flags with -mhtm basically means libitm can only really run on a power8 system instead of the default -mcpu system. David, do you prefer reverting the above hunk from the Power HTM patch or should I add the associated -mno-* options to the building of libitm? Peter
Re: [PATCH] S/390: Hardware transactional memory support
On Fri, Jun 21, 2013 at 12:23:14PM +0200, Andreas Krebbel wrote: > the attached patch implements support for hardware transactional > memory in the S/390 backend. > > The transactional execution facility has been made available with IBM > Enterprise EC12. Documentation can be found in the Principles of > Operation manual: > http://publibfi.boulder.ibm.com/epubs/pdf/dz9zr009.pdf > > The patch implements a set of GCC style builtins as well as the > builtins provided with the IBM XL compiler for compatibility reasons. > > The GCC builtins are used to implement HTM support in libitm. All > libitm testcases succeed with this patch applied (and some libitm > bugfixes from Torvald Riegel). > > The builtins are enabled by default when compiling with -march=zEC12. > They can be disabled with the -mno-htm switch. > > Bootstrapped and regtested on s390 and s390x (configured with > --with-arch=zEC12). > > Any comments? Have you considered trying it to work even when libitm itself isn't built for zEC12 or later only? I mean, both the i?86/x86_64 and powerpc* libitm HTM don't define htm_available as unconditional true, they check in some way whether the CPU supports HTM and return true if yes, and as needed some parts or whole of libitm is compiled with some compiler option that allows the HTM instructions to be generated. I see your patch will currently error out if you have HTM builtins insode of code, -mtx and don't enable -march=zEC12, could that be changed, so that -mtx is essentially independent on the chosen CPU? Jakub
