From patchwork Fri Nov 4 23:53:43 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 123717 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 0AF86B6F84 for ; Sat, 5 Nov 2011 10:54:25 +1100 (EST) Received: (qmail 19423 invoked by alias); 4 Nov 2011 23:54:19 -0000 Received: (qmail 19405 invoked by uid 22791); 4 Nov 2011 23:54:13 -0000 X-SWARE-Spam-Status: No, hits=-7.0 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_FN, TW_TM X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 04 Nov 2011 23:53:51 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id pA4Nrp49026416 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 4 Nov 2011 19:53:51 -0400 Received: from austin.quesejoda.com (vpn-8-197.rdu.redhat.com [10.11.8.197]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id pA4NriMd002120; Fri, 4 Nov 2011 19:53:45 -0400 Message-ID: <4EB47B07.7090203@redhat.com> Date: Fri, 04 Nov 2011 18:53:43 -0500 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.1.10-1.fc14 Lightning/1.0b3pre Thunderbird/3.1.10 MIME-Version: 1.0 To: Richard Guenther CC: gcc-patches , Torvald Riegel Subject: Re: [patch] 19/n: trans-mem: compiler tree/gimple stuff References: <4EB2EACC.8050307@redhat.com> In-Reply-To: Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Richard, I am going to address your suggestions in pieces, with individual patchsets, so we can tackle the less trivial bits in separate patches. So don't worry, I'm not forgetting the rest your suggestions. Below I will address what I fix with this patch. >> +/* Nonzero in a FUNCTION_DECL means this function is the transactional >> + clone of a function - called only from inside transactions. */ >> +#define DECL_IS_TM_CLONE(NODE) \ >> + (FUNCTION_DECL_CHECK (NODE)->function_decl.tm_clone_flag) > > Why is it necessary to know whether a clone is a tm clone? How do you mean? First, there are a few pretty printing places where we dump that a function is a clone. It is easy to debug dumps when you know which function is the clone and which is the original function, since we will dump both variants at code generation time. Second, there is code in the TM lowering bits where we assert that we are not trying to lower TM clones ahead of time. And there is a check in gate_tm_init() where we specify that the entire function is a TM region if it is a clone. etc, etc. Does this answer your question? >> +static inline bool >> +is_tm_safe_or_pure (tree x) > > const_tree > >> +{ >> + return is_tm_safe (x) || is_tm_pure (x); >> +} >> + Done. > The above changes seem to belong to a different changeset and look > strange. Why would attributes ever appear in two different tables? This was a recent patch by Torvald. I will ask him, but nevertheless, I can submit this separately. >> -static hashval_t >> +hashval_t >> struct_ptr_hash (const void *a) >> { >> const void * const * x = (const void * const *) a; > > Rather than exporting those here consider moving them to a common > header as inline functions. > > const void * const * x = (const void * const *) a; > return (size_t)*x>> 4; > > and on the way change that to (intptr_t)*x>> 4 Done, done. >> +&& DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL >> +&& DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_START >> + /* Check we're referring to Intel's TM specifications. */ >> +&& !strcmp (IDENTIFIER_POINTER (DECL_NAME (fn)), >> + "__builtin__ITM_beginTransaction") > > Huh. Are there others that would use the same builtin? Overly cautious. Agreed, no need for this. Removed. > >> +&& gimple_call_num_args (gs)> 0 >> + ) > > ) goes to the previouys line. Fixed. >> + case BUILT_IN_TM_LOG_M64: >> + case BUILT_IN_TM_LOG_M128: >> + case BUILT_IN_TM_LOG_M256: >> + flags |= ECF_TM_OPS; >> + break; >> + default: >> + break; >> + } >> + } > > This should not be in special_function_p which is solely to check > for the identifiers. Instead the caller of this function should handle > the builtin codes (flags_from_decl_or_type). > >> + if (DECL_NAME (fndecl) >> && IDENTIFIER_LENGTH (DECL_NAME (fndecl))<= 17 >> /* Exclude functions not at the file scope, or not `extern', >> since they are not the magic functions we would otherwise >> @@ -644,6 +697,9 @@ flags_from_decl_or_type (const_tree exp) >> if (TREE_NOTHROW (exp)) >> flags |= ECF_NOTHROW; >> >> + if (DECL_IS_TM_CLONE (exp)) >> + flags |= ECF_TM_OPS; >> + > > Thus, here. Fixed. > As you are changing features of this walker you should update its > documentation. Fixed. >> -static const_tree >> +const_tree >> strip_invariant_refs (const_tree op) >> { >> while (handled_component_p (op)) > > If you export this please move it to tree.c. > >> @@ -3085,6 +3153,8 @@ get_call_expr_in (tree t) >> t = TREE_OPERAND (t, 1); >> if (TREE_CODE (t) == WITH_SIZE_EXPR) >> t = TREE_OPERAND (t, 0); >> + if (TREE_CODE (t) == VIEW_CONVERT_EXPR) >> + t = TREE_OPERAND (t, 0); >> if (TREE_CODE (t) == CALL_EXPR) >> return t; >> return NULL_TREE; > > An unused function. Please move it to where you need it instead, > make it static and adjust it in a way to do exactly what you want. > After the above change it looks strange - handling V_C_E but > not other component refs. I see no reference to this function anywhere in the compiler (including trans-mem.*). I have removed this everywhere in the compiler I am committing the following, after having bootstrapped and regtested all languages on x86-64. * gimple.c (walk_gimple_seq): Document usage of removed_stmt field. (get_call_expr_in): Remove. (strip_invariant_refs): Move from here... * tree.c (strip_invariant_refs): ...to here. * gimple-pretty-print.c (dump_gimple_call): Remove explicit check for __builtin_ITM_beginTransaction identifier. * tree-eh.c (struct_ptr_eq): Make inline and move to tree.h. (struct_ptr_hash): Same. * gimple.h (get_call_expr_in): Remove prototype. (strip_invariant_refs): Move from here... * tree.h (strip_invariant_refs): ...to here. (is_tm_safe_or_pure): Make argument const_tree. * tree-inline.c (gimple_expand_calls_inline): Remove reference to get_call_expr_in in comment. Index: tree.c =================================================================== --- tree.c (revision 180772) +++ tree.c (working copy) @@ -11145,6 +11145,37 @@ tree_strip_sign_nop_conversions (tree ex return exp; } +/* Strip out all handled components that produce invariant + offsets. */ + +const_tree +strip_invariant_refs (const_tree op) +{ + while (handled_component_p (op)) + { + switch (TREE_CODE (op)) + { + case ARRAY_REF: + case ARRAY_RANGE_REF: + if (!is_gimple_constant (TREE_OPERAND (op, 1)) + || TREE_OPERAND (op, 2) != NULL_TREE + || TREE_OPERAND (op, 3) != NULL_TREE) + return NULL; + break; + + case COMPONENT_REF: + if (TREE_OPERAND (op, 2) != NULL_TREE) + return NULL; + break; + + default:; + } + op = TREE_OPERAND (op, 0); + } + + return op; +} + static GTY(()) tree gcc_eh_personality_decl; /* Return the GCC personality function decl. */ Index: tree.h =================================================================== --- tree.h (revision 180772) +++ tree.h (working copy) @@ -5193,6 +5193,7 @@ extern bool auto_var_in_fn_p (const_tree extern tree build_low_bits_mask (tree, unsigned); extern tree tree_strip_nop_conversions (tree); extern tree tree_strip_sign_nop_conversions (tree); +extern const_tree strip_invariant_refs (const_tree); extern tree lhd_gcc_personality (void); extern void assign_assembler_name_if_neeeded (tree); extern void warn_deprecated_use (tree, tree); @@ -5217,8 +5218,25 @@ extern void expand_return (tree); /* In tree-eh.c */ extern void using_eh_for_cleanups (void); -extern int struct_ptr_eq (const void *, const void *); -extern hashval_t struct_ptr_hash (const void *); + +/* Compare and hash for any structure which begins with a canonical + pointer. Assumes all pointers are interchangeable, which is sort + of already assumed by gcc elsewhere IIRC. */ + +static inline int +struct_ptr_eq (const void *a, const void *b) +{ + const void * const * x = (const void * const *) a; + const void * const * y = (const void * const *) b; + return *x == *y; +} + +static inline hashval_t +struct_ptr_hash (const void *a) +{ + const void * const * x = (const void * const *) a; + return (intptr_t)*x >> 4; +} /* In fold-const.c */ @@ -5864,7 +5882,7 @@ extern void record_tm_replacement (tree, extern void tm_malloc_replacement (tree); static inline bool -is_tm_safe_or_pure (tree x) +is_tm_safe_or_pure (const_tree x) { return is_tm_safe (x) || is_tm_pure (x); } Index: tree-eh.c =================================================================== --- tree-eh.c (revision 180772) +++ tree-eh.c (working copy) @@ -54,26 +54,6 @@ using_eh_for_cleanups (void) /* Misc functions used in this file. */ -/* Compare and hash for any structure which begins with a canonical - pointer. Assumes all pointers are interchangeable, which is sort - of already assumed by gcc elsewhere IIRC. */ - -int -struct_ptr_eq (const void *a, const void *b) -{ - const void * const * x = (const void * const *) a; - const void * const * y = (const void * const *) b; - return *x == *y; -} - -hashval_t -struct_ptr_hash (const void *a) -{ - const void * const * x = (const void * const *) a; - return (size_t)*x >> 4; -} - - /* Remember and lookup EH landing pad data for arbitrary statements. Really this means any statement that could_throw_p. We could stuff this information into the stmt_ann data structure, but: Index: gimple-pretty-print.c =================================================================== --- gimple-pretty-print.c (revision 180772) +++ gimple-pretty-print.c (working copy) @@ -706,11 +706,7 @@ dump_gimple_call (pretty_printer *buffer if (TREE_CODE (fn) == FUNCTION_DECL && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL && DECL_FUNCTION_CODE (fn) == BUILT_IN_TM_START - /* Check we're referring to Intel's TM specifications. */ - && !strcmp (IDENTIFIER_POINTER (DECL_NAME (fn)), - "__builtin__ITM_beginTransaction") - && gimple_call_num_args (gs) > 0 - ) + && gimple_call_num_args (gs) > 0) { tree t = gimple_call_arg (gs, 0); unsigned HOST_WIDE_INT props; Index: calls.c =================================================================== --- calls.c (revision 180772) +++ calls.c (working copy) @@ -496,60 +496,7 @@ emit_call_1 (rtx funexp, tree fntree ATT static int special_function_p (const_tree fndecl, int flags) { - if (fndecl == NULL) - return flags; - - if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) - { - switch (DECL_FUNCTION_CODE (fndecl)) - { - case BUILT_IN_TM_COMMIT: - case BUILT_IN_TM_COMMIT_EH: - case BUILT_IN_TM_ABORT: - case BUILT_IN_TM_IRREVOCABLE: - case BUILT_IN_TM_GETTMCLONE_IRR: - case BUILT_IN_TM_MEMCPY: - case BUILT_IN_TM_MEMMOVE: - case BUILT_IN_TM_MEMSET: - CASE_BUILT_IN_TM_STORE (1): - CASE_BUILT_IN_TM_STORE (2): - CASE_BUILT_IN_TM_STORE (4): - CASE_BUILT_IN_TM_STORE (8): - CASE_BUILT_IN_TM_STORE (FLOAT): - CASE_BUILT_IN_TM_STORE (DOUBLE): - CASE_BUILT_IN_TM_STORE (LDOUBLE): - CASE_BUILT_IN_TM_STORE (M64): - CASE_BUILT_IN_TM_STORE (M128): - CASE_BUILT_IN_TM_STORE (M256): - CASE_BUILT_IN_TM_LOAD (1): - CASE_BUILT_IN_TM_LOAD (2): - CASE_BUILT_IN_TM_LOAD (4): - CASE_BUILT_IN_TM_LOAD (8): - CASE_BUILT_IN_TM_LOAD (FLOAT): - CASE_BUILT_IN_TM_LOAD (DOUBLE): - CASE_BUILT_IN_TM_LOAD (LDOUBLE): - CASE_BUILT_IN_TM_LOAD (M64): - CASE_BUILT_IN_TM_LOAD (M128): - CASE_BUILT_IN_TM_LOAD (M256): - case BUILT_IN_TM_LOG: - case BUILT_IN_TM_LOG_1: - case BUILT_IN_TM_LOG_2: - case BUILT_IN_TM_LOG_4: - case BUILT_IN_TM_LOG_8: - case BUILT_IN_TM_LOG_FLOAT: - case BUILT_IN_TM_LOG_DOUBLE: - case BUILT_IN_TM_LOG_LDOUBLE: - case BUILT_IN_TM_LOG_M64: - case BUILT_IN_TM_LOG_M128: - case BUILT_IN_TM_LOG_M256: - flags |= ECF_TM_OPS; - break; - default: - break; - } - } - - if (DECL_NAME (fndecl) + if (fndecl && DECL_NAME (fndecl) && IDENTIFIER_LENGTH (DECL_NAME (fndecl)) <= 17 /* Exclude functions not at the file scope, or not `extern', since they are not the magic functions we would otherwise @@ -664,6 +611,69 @@ alloca_call_p (const_tree exp) return false; } +/* Return TRUE if FNDECL is either a TM builtin or a TM cloned + function. Return FALSE otherwise. */ + +static bool +is_tm_builtin (const_tree fndecl) +{ + if (fndecl == NULL) + return false; + + if (DECL_IS_TM_CLONE (fndecl)) + return true; + + if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) + { + switch (DECL_FUNCTION_CODE (fndecl)) + { + case BUILT_IN_TM_COMMIT: + case BUILT_IN_TM_COMMIT_EH: + case BUILT_IN_TM_ABORT: + case BUILT_IN_TM_IRREVOCABLE: + case BUILT_IN_TM_GETTMCLONE_IRR: + case BUILT_IN_TM_MEMCPY: + case BUILT_IN_TM_MEMMOVE: + case BUILT_IN_TM_MEMSET: + CASE_BUILT_IN_TM_STORE (1): + CASE_BUILT_IN_TM_STORE (2): + CASE_BUILT_IN_TM_STORE (4): + CASE_BUILT_IN_TM_STORE (8): + CASE_BUILT_IN_TM_STORE (FLOAT): + CASE_BUILT_IN_TM_STORE (DOUBLE): + CASE_BUILT_IN_TM_STORE (LDOUBLE): + CASE_BUILT_IN_TM_STORE (M64): + CASE_BUILT_IN_TM_STORE (M128): + CASE_BUILT_IN_TM_STORE (M256): + CASE_BUILT_IN_TM_LOAD (1): + CASE_BUILT_IN_TM_LOAD (2): + CASE_BUILT_IN_TM_LOAD (4): + CASE_BUILT_IN_TM_LOAD (8): + CASE_BUILT_IN_TM_LOAD (FLOAT): + CASE_BUILT_IN_TM_LOAD (DOUBLE): + CASE_BUILT_IN_TM_LOAD (LDOUBLE): + CASE_BUILT_IN_TM_LOAD (M64): + CASE_BUILT_IN_TM_LOAD (M128): + CASE_BUILT_IN_TM_LOAD (M256): + case BUILT_IN_TM_LOG: + case BUILT_IN_TM_LOG_1: + case BUILT_IN_TM_LOG_2: + case BUILT_IN_TM_LOG_4: + case BUILT_IN_TM_LOG_8: + case BUILT_IN_TM_LOG_FLOAT: + case BUILT_IN_TM_LOG_DOUBLE: + case BUILT_IN_TM_LOG_LDOUBLE: + case BUILT_IN_TM_LOG_M64: + case BUILT_IN_TM_LOG_M128: + case BUILT_IN_TM_LOG_M256: + return true; + default: + break; + } + } + return false; +} + /* Detect flags (function attributes) from the function decl or type node. */ int @@ -697,7 +707,7 @@ flags_from_decl_or_type (const_tree exp) if (TREE_NOTHROW (exp)) flags |= ECF_NOTHROW; - if (DECL_IS_TM_CLONE (exp)) + if (is_tm_builtin (exp)) flags |= ECF_TM_OPS; flags = special_function_p (exp, flags); Index: tree-inline.c =================================================================== --- tree-inline.c (revision 180772) +++ tree-inline.c (working copy) @@ -4054,9 +4054,7 @@ expand_call_inline (basic_block bb, gimp /* Expand call statements reachable from STMT_P. We can only have CALL_EXPRs as the "toplevel" tree code or nested - in a MODIFY_EXPR. See gimple.c:get_call_expr_in(). We can - unfortunately not use that function here because we need a pointer - to the CALL_EXPR, not the tree itself. */ + in a MODIFY_EXPR. */ static bool gimple_expand_calls_inline (basic_block bb, copy_body_data *id) Index: gimple.c =================================================================== --- gimple.c (revision 180772) +++ gimple.c (working copy) @@ -1341,9 +1341,11 @@ gimple_seq_copy (gimple_seq src) /* Walk all the statements in the sequence SEQ calling walk_gimple_stmt on each one. WI is as in walk_gimple_stmt. - If walk_gimple_stmt returns non-NULL, the walk is stopped, the - value is stored in WI->CALLBACK_RESULT and the statement that - produced the value is returned. + If walk_gimple_stmt returns non-NULL, the walk is stopped, and the + value is stored in WI->CALLBACK_RESULT. Also, the statement that + produced the value is returned if this statement has not been + removed by a callback (wi->removed_stmt). If the statement has + been removed, NULL is returned. Otherwise, all the statements are walked and NULL returned. */ @@ -2850,37 +2852,6 @@ is_gimple_address (const_tree t) } } -/* Strip out all handled components that produce invariant - offsets. */ - -const_tree -strip_invariant_refs (const_tree op) -{ - while (handled_component_p (op)) - { - switch (TREE_CODE (op)) - { - case ARRAY_REF: - case ARRAY_RANGE_REF: - if (!is_gimple_constant (TREE_OPERAND (op, 1)) - || TREE_OPERAND (op, 2) != NULL_TREE - || TREE_OPERAND (op, 3) != NULL_TREE) - return NULL; - break; - - case COMPONENT_REF: - if (TREE_OPERAND (op, 2) != NULL_TREE) - return NULL; - break; - - default:; - } - op = TREE_OPERAND (op, 0); - } - - return op; -} - /* Return true if T is a gimple invariant address. */ bool @@ -3143,23 +3114,6 @@ is_gimple_mem_ref_addr (tree t) || decl_address_invariant_p (TREE_OPERAND (t, 0))))); } -/* If T makes a function call, return the corresponding CALL_EXPR operand. - Otherwise, return NULL_TREE. */ - -tree -get_call_expr_in (tree t) -{ - if (TREE_CODE (t) == MODIFY_EXPR) - t = TREE_OPERAND (t, 1); - if (TREE_CODE (t) == WITH_SIZE_EXPR) - t = TREE_OPERAND (t, 0); - if (TREE_CODE (t) == VIEW_CONVERT_EXPR) - t = TREE_OPERAND (t, 0); - if (TREE_CODE (t) == CALL_EXPR) - return t; - return NULL_TREE; -} - /* Given a memory reference expression T, return its base address. The base address of a memory reference expression is the main Index: gimple.h =================================================================== --- gimple.h (revision 180772) +++ gimple.h (working copy) @@ -1015,8 +1015,6 @@ extern bool is_gimple_non_addressable (t /* Returns true iff T is a valid call address expression. */ extern bool is_gimple_call_addr (tree); -/* If T makes a function call, returns the CALL_EXPR operand. */ -extern tree get_call_expr_in (tree t); extern void recalculate_side_effects (tree); extern bool gimple_compare_field_offset (tree, tree); @@ -1037,7 +1035,6 @@ extern bool walk_stmt_load_store_ops (gi bool (*)(gimple, tree, void *), bool (*)(gimple, tree, void *)); extern bool gimple_ior_addresses_taken (bitmap, gimple); -extern const_tree strip_invariant_refs (const_tree); extern bool gimple_call_builtin_p (gimple, enum built_in_function); extern bool gimple_asm_clobbers_memory_p (const_gimple);