* [RFC, tentative patch] Adjust cost for conversion expression @ 2016-11-24 10:58 Pitchumani Sivanupandi 2016-11-24 11:24 ` Richard Biener 2016-11-28 22:40 ` Jeff Law 0 siblings, 2 replies; 5+ messages in thread From: Pitchumani Sivanupandi @ 2016-11-24 10:58 UTC (permalink / raw) To: rguenther, law, GCC Patches [-- Attachment #1: Type: text/plain, Size: 804 bytes --] GCC inlines small functions if the code size after expansion is not excedded. For test case (inline.c, avr-gcc -Os -S inline.c) code size become higher if 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are considered as zero cost expression. Few conversions will cost additional instructions. For targets like AVR it will cost considerably as it's register size is just one byte. Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost to 1 if the LHS is bigger than RHS and target's word_mode. Is this Ok? Would it be reasonable if cost evaluated as below instead of constant 1? if (LHS PRECISION > RHS PRECISION) cost = LHS_PRECISION / word_mode - 1 else cost = 0 Built GCC for native with bootstrap enabled. No issues. Regards, Pitchumani [-- Attachment #2: inline.c --] [-- Type: text/x-csrc, Size: 422 bytes --] typedef unsigned int uint16_t __attribute__((__mode__(__HI__))); typedef unsigned int uint32_t __attribute__ ((__mode__ (__SI__))); char c = 12; volatile long l = 1; volatile uint32_t (*p)(); static uint16_t bar () { return c; } static uint32_t foo () { return ((uint32_t)bar() << 16 | bar()); } __attribute__((noinline)) void func2 (uint32_t(*ptr)()) { p = ptr(); l = foo(); } void main () { func2(foo); } [-- Attachment #3: adjust-conversion-cost.patch --] [-- Type: text/x-patch, Size: 2150 bytes --] diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 6899d2a..dbb305d 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3867,19 +3867,28 @@ estimate_move_cost (tree type, bool ARG_UNUSED (speed_p)) static int estimate_operator_cost (enum tree_code code, eni_weights *weights, - tree op1 ATTRIBUTE_UNUSED, tree op2) + tree op1, tree op2) { + unsigned int lhs_prec, rhs_prec; switch (code) { /* These are "free" conversions, or their presumed cost is folded into other operations. */ case RANGE_EXPR: - CASE_CONVERT: case COMPLEX_EXPR: case PAREN_EXPR: case VIEW_CONVERT_EXPR: return 0; - + CASE_CONVERT: + { + if (op1 && op2) { + lhs_prec = element_precision (TREE_TYPE (op1)); + rhs_prec = element_precision (TREE_TYPE (op2)); + if ((lhs_prec > rhs_prec) && (lhs_prec > word_mode)) + return 1; + } + return 0; + } /* Assign cost of 1 to usual operations. ??? We may consider mapping RTL costs to this. */ case COND_EXPR: @@ -4026,6 +4035,7 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) { unsigned cost, i; enum gimple_code code = gimple_code (stmt); + enum tree_code rhs_code; tree lhs; tree rhs; @@ -4064,9 +4074,17 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) if (gimple_assign_load_p (stmt)) cost += estimate_move_cost (TREE_TYPE (rhs), weights->time_based); - cost += estimate_operator_cost (gimple_assign_rhs_code (stmt), weights, + rhs_code = gimple_assign_rhs_code (stmt); + if ((rhs_code == NOP_EXPR) || (rhs_code == CONVERT_EXPR)) + { + cost += estimate_operator_cost (rhs_code, weights, + gimple_assign_lhs (stmt), + gimple_assign_rhs1 (stmt)); + } + else + cost += estimate_operator_cost (rhs_code, weights, gimple_assign_rhs1 (stmt), - get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) + get_gimple_rhs_class (rhs_code) == GIMPLE_BINARY_RHS ? gimple_assign_rhs2 (stmt) : NULL); break; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC, tentative patch] Adjust cost for conversion expression 2016-11-24 10:58 [RFC, tentative patch] Adjust cost for conversion expression Pitchumani Sivanupandi @ 2016-11-24 11:24 ` Richard Biener 2016-11-28 13:10 ` Pitchumani Sivanupandi 2016-11-28 22:40 ` Jeff Law 1 sibling, 1 reply; 5+ messages in thread From: Richard Biener @ 2016-11-24 11:24 UTC (permalink / raw) To: Pitchumani Sivanupandi; +Cc: law, GCC Patches On Thu, 24 Nov 2016, Pitchumani Sivanupandi wrote: > GCC inlines small functions if the code size after expansion is not excedded. > For test case (inline.c, avr-gcc -Os -S inline.c) code size become higher if > 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are > considered > as zero cost expression. > > Few conversions will cost additional instructions. For targets like AVR > it will cost considerably as it's register size is just one byte. > > Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost > to 1 if the LHS is bigger than RHS and target's word_mode. > > Is this Ok? > > Would it be reasonable if cost evaluated as below instead of constant 1? > if (LHS PRECISION > RHS PRECISION) > cost = LHS_PRECISION / word_mode - 1 > else > cost = 0 > > Built GCC for native with bootstrap enabled. No issues. I believe a better check would be tree_nop_conversion_p (). Thus CASE_CONVERT: return tree_nop_conversion_p (type, TREE_TYPE (op0)) ? 0 : 1; note that + rhs_code = gimple_assign_rhs_code (stmt); + if ((rhs_code == NOP_EXPR) || (rhs_code == CONVERT_EXPR)) + { + cost += estimate_operator_cost (rhs_code, weights, + gimple_assign_lhs (stmt), + gimple_assign_rhs1 (stmt)); + } is super-ugly - please simply add the type of the expression as an additional argument (see gimple_expr_type (), but the type of the LHS would do as well). Note that doing this change might require some re-tuning of inliner params, but otherwise it's totally sensible. Thanks, Richard. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC, tentative patch] Adjust cost for conversion expression 2016-11-24 11:24 ` Richard Biener @ 2016-11-28 13:10 ` Pitchumani Sivanupandi 2016-11-28 13:36 ` Richard Biener 0 siblings, 1 reply; 5+ messages in thread From: Pitchumani Sivanupandi @ 2016-11-28 13:10 UTC (permalink / raw) To: Richard Biener; +Cc: law, GCC Patches [-- Attachment #1: Type: text/plain, Size: 2198 bytes --] On Thursday 24 November 2016 04:54 PM, Richard Biener wrote: > On Thu, 24 Nov 2016, Pitchumani Sivanupandi wrote: > >> GCC inlines small functions if the code size after expansion is not excedded. >> For test case (inline.c, avr-gcc -Os -S inline.c) code size become higher if >> 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are >> considered >> as zero cost expression. >> >> Few conversions will cost additional instructions. For targets like AVR >> it will cost considerably as it's register size is just one byte. >> >> Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost >> to 1 if the LHS is bigger than RHS and target's word_mode. >> >> Is this Ok? >> >> Would it be reasonable if cost evaluated as below instead of constant 1? >> if (LHS PRECISION > RHS PRECISION) >> cost = LHS_PRECISION / word_mode - 1 >> else >> cost = 0 >> >> Built GCC for native with bootstrap enabled. No issues. > I believe a better check would be tree_nop_conversion_p (). Thus > > CASE_CONVERT: > return tree_nop_conversion_p (type, TREE_TYPE (op0)) ? 0 : 1; > > note that > > + rhs_code = gimple_assign_rhs_code (stmt); > + if ((rhs_code == NOP_EXPR) || (rhs_code == CONVERT_EXPR)) > + { > + cost += estimate_operator_cost (rhs_code, weights, > + gimple_assign_lhs (stmt), > + gimple_assign_rhs1 (stmt)); > + } > > is super-ugly - please simply add the type of the expression as an > additional argument (see gimple_expr_type (), but the type of the > LHS would do as well). > > Note that doing this change might require some re-tuning of > inliner params, but otherwise it's totally sensible. Thanks. Attached the revised patch. When reg-tested for x86_64 found following failures. FAIL: gcc.dg/uninit-19.c FAIL: gcc.dg/vect/vect-104.c For uninit-19.c, index to dereference float array is converted to long unsigned int and that is not tree_nop_conversion_p. This caused function cost to increase and auto inline is rejected. I think, this may be huge penalty for target like x86_64 which has rich ISA. Any suggestions to avoid hitting such targets? Regards, Pitchumani [-- Attachment #2: adjust-conversion-cost-rev2.patch --] [-- Type: text/x-patch, Size: 1819 bytes --] diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index 6899d2a..e9f45be 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -3867,19 +3867,19 @@ estimate_move_cost (tree type, bool ARG_UNUSED (speed_p)) static int estimate_operator_cost (enum tree_code code, eni_weights *weights, - tree op1 ATTRIBUTE_UNUSED, tree op2) + tree op1, tree op2, tree op0) { switch (code) { /* These are "free" conversions, or their presumed cost is folded into other operations. */ case RANGE_EXPR: - CASE_CONVERT: case COMPLEX_EXPR: case PAREN_EXPR: case VIEW_CONVERT_EXPR: return 0; - + CASE_CONVERT: + return tree_nop_conversion_p (op0, TREE_TYPE (op1)) ? 0 : 1; /* Assign cost of 1 to usual operations. ??? We may consider mapping RTL costs to this. */ case COND_EXPR: @@ -4068,13 +4068,14 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) gimple_assign_rhs1 (stmt), get_gimple_rhs_class (gimple_assign_rhs_code (stmt)) == GIMPLE_BINARY_RHS - ? gimple_assign_rhs2 (stmt) : NULL); + ? gimple_assign_rhs2 (stmt) : NULL, + gimple_expr_type (stmt)); break; case GIMPLE_COND: cost = 1 + estimate_operator_cost (gimple_cond_code (stmt), weights, gimple_op (stmt, 0), - gimple_op (stmt, 1)); + gimple_op (stmt, 1), gimple_expr_type (stmt)); break; case GIMPLE_SWITCH: @@ -4129,7 +4130,7 @@ estimate_num_insns (gimple *stmt, eni_weights *weights) &dconst2))) return estimate_operator_cost (MULT_EXPR, weights, gimple_call_arg (stmt, 0), - gimple_call_arg (stmt, 0)); + gimple_call_arg (stmt, 0), gimple_expr_type (stmt)); break; default: ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC, tentative patch] Adjust cost for conversion expression 2016-11-28 13:10 ` Pitchumani Sivanupandi @ 2016-11-28 13:36 ` Richard Biener 0 siblings, 0 replies; 5+ messages in thread From: Richard Biener @ 2016-11-28 13:36 UTC (permalink / raw) To: Pitchumani Sivanupandi; +Cc: law, GCC Patches On Mon, 28 Nov 2016, Pitchumani Sivanupandi wrote: > On Thursday 24 November 2016 04:54 PM, Richard Biener wrote: > > On Thu, 24 Nov 2016, Pitchumani Sivanupandi wrote: > > > > > GCC inlines small functions if the code size after expansion is not > > > excedded. > > > For test case (inline.c, avr-gcc -Os -S inline.c) code size become higher > > > if > > > 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are > > > considered > > > as zero cost expression. > > > > > > Few conversions will cost additional instructions. For targets like AVR > > > it will cost considerably as it's register size is just one byte. > > > > > > Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost > > > to 1 if the LHS is bigger than RHS and target's word_mode. > > > > > > Is this Ok? > > > > > > Would it be reasonable if cost evaluated as below instead of constant 1? > > > if (LHS PRECISION > RHS PRECISION) > > > cost = LHS_PRECISION / word_mode - 1 > > > else > > > cost = 0 > > > > > > Built GCC for native with bootstrap enabled. No issues. > > I believe a better check would be tree_nop_conversion_p (). Thus > > > > CASE_CONVERT: > > return tree_nop_conversion_p (type, TREE_TYPE (op0)) ? 0 : 1; > > > > note that > > > > + rhs_code = gimple_assign_rhs_code (stmt); > > + if ((rhs_code == NOP_EXPR) || (rhs_code == CONVERT_EXPR)) > > + { > > + cost += estimate_operator_cost (rhs_code, weights, > > + gimple_assign_lhs (stmt), > > + gimple_assign_rhs1 (stmt)); > > + } > > > > is super-ugly - please simply add the type of the expression as an > > additional argument (see gimple_expr_type (), but the type of the > > LHS would do as well). > > > > Note that doing this change might require some re-tuning of > > inliner params, but otherwise it's totally sensible. > > Thanks. Attached the revised patch. > > When reg-tested for x86_64 found following failures. > FAIL: gcc.dg/uninit-19.c > FAIL: gcc.dg/vect/vect-104.c > > For uninit-19.c, index to dereference float array is converted to > long unsigned int and that is not tree_nop_conversion_p. This caused > function cost to increase and auto inline is rejected. Hum, yeah... so the simple tree_nop_conversion_p is too coarse... (the conversions do result in code generation though). > I think, this may be huge penalty for target like x86_64 which has rich ISA. > Any suggestions to avoid hitting such targets? No good one. estimate_num_insns is really just a wild guess ... but one with a huge impact downstream. Note we're not trying to be clever anywhere but we even do not handle "gross" mistakes like counting vector additions of 2048 element vectors (later split to HW supportable sizes) as one. Yes, we're trying to be clever at call stmts, but that's it. Richard. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC, tentative patch] Adjust cost for conversion expression 2016-11-24 10:58 [RFC, tentative patch] Adjust cost for conversion expression Pitchumani Sivanupandi 2016-11-24 11:24 ` Richard Biener @ 2016-11-28 22:40 ` Jeff Law 1 sibling, 0 replies; 5+ messages in thread From: Jeff Law @ 2016-11-28 22:40 UTC (permalink / raw) To: Pitchumani Sivanupandi, rguenther, GCC Patches On 11/24/2016 03:58 AM, Pitchumani Sivanupandi wrote: > GCC inlines small functions if the code size after expansion is not > excedded. > For test case (inline.c, avr-gcc -Os -S inline.c) code size become > higher if > 'func2' is inlined. It happens because the CONVERT_EXPR/ NOP_EXPR are > considered > as zero cost expression. > > Few conversions will cost additional instructions. For targets like AVR > it will cost considerably as it's register size is just one byte. > > Attached the tentative patch that changes the CONVERT_EXPR/ NOP_EXPR cost > to 1 if the LHS is bigger than RHS and target's word_mode. > > Is this Ok? > > Would it be reasonable if cost evaluated as below instead of constant 1? > if (LHS PRECISION > RHS PRECISION) > cost = LHS_PRECISION / word_mode - 1 > else > cost = 0 > > Built GCC for native with bootstrap enabled. No issues. My high level worry here is that this introduces a target dependency into the early part of the tree optimization pipeline (the test against word_mode). I'd be more open to giving a cost to any widening conversion. That kind of change might have a wide reaching impact though. jeff ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-28 22:40 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-24 10:58 [RFC, tentative patch] Adjust cost for conversion expression Pitchumani Sivanupandi 2016-11-24 11:24 ` Richard Biener 2016-11-28 13:10 ` Pitchumani Sivanupandi 2016-11-28 13:36 ` Richard Biener 2016-11-28 22:40 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).