This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
| Other format: | [Raw text] | |
On 11/07/13 09:09, Martin Jambor wrote:
I am glad this is becoming a useful infrastructure rather than just a part of IPA-SRA. Note that while ipa_combine_adjustments is not used from anywhere and thus probably buggy anyway, it should in theory be able to process new_param adjustments too. Can you please at least put a "not implemented" assert there? (The reason is that the plan
Done.
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index c38ba82..faae080 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -446,6 +446,13 @@ determine_versionability (struct cgraph_node *node) reason = "not a tree_versionable_function"; else if (cgraph_function_body_availability (node) <= AVAIL_OVERWRITABLE) reason = "insufficient body availability"; + else if (node->has_simd_clones) + { + /* Ideally we should clone the SIMD clones themselves and create + vector copies of them, so IPA-cp and SIMD clones can happily + coexist, but that may not be worth the effort. */ + reason = "function has SIMD clones"; + }Lets hope we will eventually fix this in some followup :-)
Sure, but to be honest it's not super high on my priority list, perhaps once the basic functionality is in trunk.
+ tree new_arg_types = NULL; + for (int i = 0; i < len; i++) { struct ipa_parm_adjustment *adj; gcc_assert (link); adj = &adjustments[i]; - parm = oparms[adj->base_index]; + tree parm; + if (adj->new_param)I don't know what I was thinking when I invented copy_param and remove_param as multiple flags rather than a single enum, I probably wasn't thinking at all. I can change it myself as a followup if you have more pressing tasks now. Meanwhile, can you gcc_checking_assert that at most one flag is set at appropriate places?
Not a problem, I can implement the enum changes since I'm already changing all this code. Done.
+ parm = NULL; + else + parm = oparms[adj->base_index]; adj->base = parm;I do not think it makes sense for new parameters to have a base which is basically the old decl. Do you have any reasons for not setting it to NULL?
In this particular case, adj->base is already being set to NULL because parm=NULL for adj->op. The code now reads:
if (adj->op == IPA_PARM_OP_NEW)
parm = NULL;
else
parm = oparms[adj->base_index];
adj->base = parm;
Am I missing something? Base is already been set to NULL for new parameters.
if (adj->copy_param) @@ -3417,8 +3418,18 @@ ipa_modify_formal_parameters (tree fndecl, ipa_parm_adjustment_vec adjustments, tree new_parm; tree ptype;- if (adj->by_ref) - ptype = build_pointer_type (adj->type);Please add gcc_checking_assert (!adj->by_ref || adj->simdlen == 0) here...
Done.
+ const char *prefix + = adj->new_param ? adj->new_arg_prefix : synth_parm_prefix;Can we perhaps get rid of synth_parm_prefix then and just have adj->new_arg_prefix? It's not particularly important but this is weird.
Done.
+ DECL_NAME (new_parm) = create_tmp_var_name (prefix); DECL_ARTIFICIAL (new_parm) = 1; DECL_ARG_TYPE (new_parm) = ptype; DECL_CONTEXT (new_parm) = fndecl; @@ -3436,17 +3448,20 @@ ipa_modify_formal_parameters (tree fndecl, ipa_parm_adjustment_vec adjustments, DECL_IGNORED_P (new_parm) = 1; layout_decl (new_parm, 0); - adj->base = parm; + if (adj->new_param) + adj->base = new_parm;Again, shouldn't this be NULL?
This one, yes :). Done.
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 48634d2..8d7d9b9 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -634,9 +634,10 @@ struct ipa_parm_adjustment arguments. */ tree alias_ptr_type; - /* The new declaration when creating/replacing a parameter. Created by - ipa_modify_formal_parameters, useful for functions modifying the body - accordingly. */ + /* The new declaration when creating/replacing a parameter. Created + by ipa_modify_formal_parameters, useful for functions modifying + the body accordingly. For brand new arguments, this is the newly + created argument. */ tree reduction;We should eventually rename this to new_decl or something, given that this is not an SRA thing any more. But that can be done later.
Done.
/* New declaration of a substitute variable that we may use to replace all @@ -647,15 +648,36 @@ struct ipa_parm_adjustment is NULL), this is going to be its nonlocalized vars value. */ tree nonlocal_value; + /* If this is a brand new argument, this holds the prefix to be used + for the DECL_NAME. */ + const char *new_arg_prefix; + /* Offset into the original parameter (for the cases when the new parameter is a component of an original one). */ HOST_WIDE_INT offset; - /* Zero based index of the original parameter this one is based on. (ATM - there is no way to insert a new parameter out of the blue because there is - no need but if it arises the code can be easily exteded to do so.) */ + /* Zero based index of the original parameter this one is based on. */ int base_index; + /* If non-null, the parameter is a vector of `type' with this many + elements. */ + int simdlen; + + /* This is a brand new parameter. + + For new parameters, base_index must be >= the number of + DECL_ARGUMENTS in the function. That is, new arguments will be + the last arguments in the adjusted function. + + ?? Perhaps we could redesign ipa_modify_formal_parameters() to + reorganize argument position, thus allowing inserting of brand + new arguments anywhere, but there is no use for this now.Where does this requirement come from? At least at the moment I cannot see why ipa_modify_formal_parameters wouldn't be able to reorder parameters as it is? What breaks if base_index of adjustments for new parameters has zero or a nonsensical value?
From my very vivid imagination. Forget I said that. I hadn't looked into it at all; I just assumed. I have removed the ??? comment.
Hm, if you can directly use these, I really think you should rename them somehow so that their names do not contain SRA and move them to ipa-prop.c.
I'd like to do this as a followup so you can see all my changes before I move things en masse.
Thanks for reviving this slightly moribund infrastructure and sorry again for the delay,
Not a problem. Thanks for the review.Would you be so kind as to review these changes to make sure I didn't miss anything?
The patch is lightly tested as my current box is pathetically slow today but so far so good with gomp.exp tests.
OK for gomp-4_0-branch pending tests? Aldy
Attachment:
curr
Description: Text document
| Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
|---|---|---|
| Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |