Attached is Richard Henderson's patch that marks calls that start a
transaction as returns-twice. In turn, these calls will be treated like
a call to setjmp.
This was motivated by the miscompilation of one of the STAMP
applications (Genome), where a stack slot was used as temp storage for a
CPU register but not restored when the transaction got aborted and
restarted (then, after restart, the program crashed because it used
inconsistent data). With the attached patch and in this particular
example, the stack slots that are written to in the transaction do not
get read during the transaction. (-fno-caller-saves was not a
sufficient solution, BTW.)
AFAICT, we previously wanted to handle "restart safety" by adding
abnormal edges to all calls to the TM runtime library (which could in
turn call the libitm longjmp that actually restarts a transaction).
Richard mentioned that we could drop this code (I think he meant this,
and others pieces perhaps) if the returns-twice approach works. BTW,
did we also add abnormal edges to any call to transactional clones,
which could in turn call the TM runtime library?
Conceptually, what we would need to enforce is that all stack slots that
are live-in into a transaction (i.e., the live range crosses a call to
_ITM_beginTransaction) do not get reused until the matching call to
_ITM_commitTransaction. Alternatively, we can reuse those, but need to
save and restore them after aborts.
As Richard suggested, I looked at the handling of REG_SETJMP (see the
summary below). This looks like a sufficient solution to me (eg, don't
share stack slots if regs cross a setjmp) but I don't know nearly enough
about this to really understand whether it is. Can somebody else have a
look please? In particular:
- Is it really sufficient?
- Is this too conservative (e.g., no CSE at all)? If so, we should look
at this again for 4.8 I guess, otherwise every function that uses a
transaction will be slower, and slowing down nontransactional code
isn't nice. Or won't that be much of a slowdown?
- Do we potentially get unnecessary warnings (if vars are live across
a transaction begin)? I didn't get such warnings in the STAMP app
that wasn't working though. Does anyone has suggestions for a test
case?
- For the long-term, should we try to limit the setjmp effects to the
TM region? Contrary to standard setjmp/longjmp, we know about the
regions and where the longjmps matching a setjmp are. Any thoughts
on how much this might matter performance-wise?
Overall, I think we should still use it, unless we can come up with
another fix in time for 4.7.
Opinions?
Torvald
Summary:
- cprop.c: constant propagation only runs if !cfun->calls_setjmp
- gcse.c: Same.
- cse.c: we don't CSE over a call to setjmp. But comment associates this
just with some weird longjmp on VAX and other systems.
- cselib.c (cselib_process_insn): Forget everything at a setjmp.
- callers of this function are: dse_step1(), local_cprop_pass(),
reload_cse_regs(), thread_jump(), vt_initialize()
- ira-lives.c (process_bb_node_lives): ??? Don't allocate allocnos
that cross setjmps.
- regstat.c (regstat_bb_compute_ri): Marks all pseudo(?) regs that
cross a setjmp. Clients of this information:
- (regstat_compute_ri): ??? REG_LIVE_LENGTH(regno) = -1;
REG_BASIC_BLOCK(regno) = REG_BLOCK_UNKNOWN;
- function.c (generate_setjmp_warnings): Warnings if vars are live
across setjmp.
- ira-color.c (coalesce_spill_slots): ??? Don't share stack slots if
one of the regs crosses a setjmp?
- reload1.c (reload_as_needed): Don't reuse any reload reg contents
across a setjmp.
- reload.c (find_equiv_reg): Don't reuse register contents from before
a setjmp-type function call.
- resource.c (mark_referenced_resources, mark_set_resources): setjmp
uses all registers.
- sched-deps.c: setjmp acts as a MOVE_BARRIER. What does this barrier
do precisely?
commit 53e0ab5b86ca38d55e4c4fd927be33143586ad15
Author: Torvald Riegel <trie...@redhat.com>
Date: Mon Dec 19 23:36:45 2011 +0100
rth's returns-twice fix
diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def
index 619794e..c3132cc 100644
--- a/gcc/builtin-attrs.def
+++ b/gcc/builtin-attrs.def
@@ -98,6 +98,7 @@ DEF_ATTR_IDENT (ATTR_STRFTIME, "strftime")
DEF_ATTR_IDENT (ATTR_TYPEGENERIC, "type generic")
DEF_ATTR_IDENT (ATTR_TM_REGPARM, "*tm regparm")
DEF_ATTR_IDENT (ATTR_TM_TMPURE, "transaction_pure")
+DEF_ATTR_IDENT (ATTR_RETURNS_TWICE, "returns_twice")
DEF_ATTR_TREE_LIST (ATTR_NOVOPS_LIST, ATTR_NOVOPS, ATTR_NULL, ATTR_NULL)
@@ -241,6 +242,8 @@ DEF_ATTR_TREE_LIST (ATTR_TM_NORETURN_NOTHROW_LIST,
ATTR_TM_REGPARM, ATTR_NULL, ATTR_NORETURN_NOTHROW_LIST)
DEF_ATTR_TREE_LIST (ATTR_TM_CONST_NOTHROW_LIST,
ATTR_TM_REGPARM, ATTR_NULL, ATTR_CONST_NOTHROW_LIST)
+DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_RT_LIST,
+ ATTR_RETURNS_TWICE, ATTR_NULL, ATTR_TM_NOTHROW_LIST)
/* Same attributes used for BUILT_IN_MALLOC except with TM_PURE thrown in. */
DEF_ATTR_TREE_LIST (ATTR_TMPURE_MALLOC_NOTHROW_LIST,
diff --git a/gcc/gtm-builtins.def b/gcc/gtm-builtins.def
index 9fcbdb0..1630a0e 100644
--- a/gcc/gtm-builtins.def
+++ b/gcc/gtm-builtins.def
@@ -1,5 +1,5 @@
DEF_TM_BUILTIN (BUILT_IN_TM_START, "_ITM_beginTransaction",
- BT_FN_UINT_UINT, ATTR_TM_NOTHROW_LIST)
+ BT_FN_UINT_UINT, ATTR_TM_NOTHROW_RT_LIST)
DEF_TM_BUILTIN (BUILT_IN_TM_COMMIT, "_ITM_commitTransaction",
BT_FN_VOID, ATTR_TM_NOTHROW_LIST)
