| Message ID | alpine.LSU.2.11.1507271211070.19642@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
On Mon, 27 Jul 2015, Richard Biener wrote: > > I noticed that the code we generate for a simple gimple_assign_rhs1 (stmt) > is quite bad as we have two checking pieces. The implementation is now > > static inline tree > gimple_assign_rhs1 (const_gimple gs) > { > GIMPLE_CHECK (gs, GIMPLE_ASSIGN); > return gimple_op (gs, 1); > } > > and the hidden checking is due to gimple_op being > > static inline tree * > gimple_ops (gimple gs) > { > size_t off; > > /* All the tuples have their operand vector at the very bottom > of the structure. Note that those structures that do not > have an operand vector have a zero offset. */ > off = gimple_ops_offset_[gimple_statement_structure (gs)]; > gcc_gimple_checking_assert (off != 0); > > return (tree *) ((char *) gs + off); > > (ugly in itself considering that we just verified we have a gassign > which has an operand vector as member...). gimple_op incurs two > additional memory reference to compute gimple_statement_structure > and gimple_ops_offset_ plus the checking bits. > > The simplest thing would be to use > > GIMPLE_CHECK (gs, GIMPLE_ASSIGN); > return as_a <const gassign *> (gs)->op[1]; > > but that's not optimal either because we check we have a gassign > again (not optimized in stage1). So I decided to invent a fancy > as_a which reports errors similar to GIMPLE_CHECK and makes the > code look like > > const gassign *ass = GIMPLE_CHECK2<const gassign *> (gs); > return ass->op[1]; > > for some reason I needed to overload is_a_helper for const_gimple > (I thought the is_a machinery would transparently support qualified > types?). > > I'm missing a fallback for !ENABLE_GIMPLE_CHECKING as well > as an overload for 'gimple' I guess. I just changed > gimple_assign_rhs1 for now. > > So this is a RFC. > > I understand in the end the whole GCC may use gassign everywhere > we access gimple_assign_rhs1 but I don't see this happening soon > and this simple proof-of-concept patch already reduces unoptimized > text size of gimple-match.c from 836080 to 836359 (too bad) and > optimized from 585731 to 193190 bytes (yay!) on x86_64 using trunk. > Some inlining must go very differently - we just have 544 calls to > gimple_assign_rhs1 in gimple-match.c. .optimizes shows all calls > remaining as > > <bb 89>: > _ZL18gimple_assign_rhs1PK21gimple_statement_base.part.32 (def_stmt_47); > > which is > > tree_node* gimple_assign_rhs1(const_gimple) (const struct > gimple_statement_base * gs) > { > <bb 2>: > gimple_check_failed (gs_1(D), > &"/space/rguenther/tramp3d/trunk/gcc/gimple.h"[0], 2381, > &"gimple_assign_rhs1"[0], 6, 0); > > } > > so eventually we just do a better job with profile estimation. Ok, numbers are off because of a typo I introduced late. > + if (ret) > + gimple_check_failed (gs, file, line, fun, T()->code_, ERROR_MARK); should be if (!ret) of course ... Optimized code size after the patch is 588878 (we still avoid a lot of code and the checking fails are still split out). Not sure where the code size increase is from. Unoptimized code size after the patch is 836233. Richard. > I think inliner-wise we are even as we get rid of the gimple_op () > inline but instead get the GIMPLE_CHECK2 inline. > > So - any comments? > > Thanks, > Richard. > > 2015-07-27 Richard Biener <rguenther@suse.de> > > * gimple.h (GIMPLE_CHECK2): New inline template function. > (gassign::code_): New constant static member. > (is_a_helper<const gassign *>): Add. > (gimple_assign_rhs1): Use GIMPLE_CHECK2 and a cheaper way > to access operand 1. > * gimple.c (gassign::code_): Define. > > Index: gcc/gimple.h > =================================================================== > --- gcc/gimple.h (revision 226240) > +++ gcc/gimple.h (working copy) > @@ -51,6 +51,16 @@ extern void gimple_check_failed (const_g > gimple_check_failed (__gs, __FILE__, __LINE__, __FUNCTION__, \ > (CODE), ERROR_MARK); \ > } while (0) > +template <typename T> > +static inline T > +GIMPLE_CHECK2(const_gimple gs, const char *file = __builtin_FILE (), > + int line = __builtin_LINE (), const char *fun = __builtin_FUNCTION ()) > +{ > + T ret = dyn_cast <T> (gs); > + if (ret) > + gimple_check_failed (gs, file, line, fun, T()->code_, ERROR_MARK); > + return ret; > +} > #else /* not ENABLE_GIMPLE_CHECKING */ > #define gcc_gimple_checking_assert(EXPR) ((void)(0 && (EXPR))) > #define GIMPLE_CHECK(GS, CODE) (void)0 > @@ -832,6 +842,7 @@ struct GTY((tag("GSS_WITH_OPS"))) > struct GTY((tag("GSS_WITH_MEM_OPS"))) > gassign : public gimple_statement_with_memory_ops > { > + static const enum gimple_code code_ = GIMPLE_ASSIGN; > /* no additional fields; this uses the layout for GSS_WITH_MEM_OPS. */ > }; > > @@ -864,6 +875,14 @@ is_a_helper <gassign *>::test (gimple gs > template <> > template <> > inline bool > +is_a_helper <const gassign *>::test (const_gimple gs) > +{ > + return gs->code == GIMPLE_ASSIGN; > +} > + > +template <> > +template <> > +inline bool > is_a_helper <gbind *>::test (gimple gs) > { > return gs->code == GIMPLE_BIND; > @@ -2359,8 +2378,8 @@ gimple_assign_set_lhs (gimple gs, tree l > static inline tree > gimple_assign_rhs1 (const_gimple gs) > { > - GIMPLE_CHECK (gs, GIMPLE_ASSIGN); > - return gimple_op (gs, 1); > + const gassign *ass = GIMPLE_CHECK2<const gassign *> (gs); > + return ass->op[1]; > } > > > Index: gcc/gimple.c > =================================================================== > --- gcc/gimple.c (revision 226240) > +++ gcc/gimple.c (working copy) > @@ -89,6 +89,10 @@ static const char * const gimple_alloc_k > "everything else" > }; > > +/* Static gimple tuple members. */ > +const enum gimple_code gassign::code_; > + > + > /* Gimple tuple constructors. > Note: Any constructor taking a ``gimple_seq'' as a parameter, can > be passed a NULL to start with an empty sequence. */ >
Index: gcc/gimple.h =================================================================== --- gcc/gimple.h (revision 226240) +++ gcc/gimple.h (working copy) @@ -51,6 +51,16 @@ extern void gimple_check_failed (const_g gimple_check_failed (__gs, __FILE__, __LINE__, __FUNCTION__, \ (CODE), ERROR_MARK); \ } while (0) +template <typename T> +static inline T +GIMPLE_CHECK2(const_gimple gs, const char *file = __builtin_FILE (), + int line = __builtin_LINE (), const char *fun = __builtin_FUNCTION ()) +{ + T ret = dyn_cast <T> (gs); + if (ret) + gimple_check_failed (gs, file, line, fun, T()->code_, ERROR_MARK); + return ret; +} #else /* not ENABLE_GIMPLE_CHECKING */ #define gcc_gimple_checking_assert(EXPR) ((void)(0 && (EXPR))) #define GIMPLE_CHECK(GS, CODE) (void)0 @@ -832,6 +842,7 @@ struct GTY((tag("GSS_WITH_OPS"))) struct GTY((tag("GSS_WITH_MEM_OPS"))) gassign : public gimple_statement_with_memory_ops { + static const enum gimple_code code_ = GIMPLE_ASSIGN; /* no additional fields; this uses the layout for GSS_WITH_MEM_OPS. */ }; @@ -864,6 +875,14 @@ is_a_helper <gassign *>::test (gimple gs template <> template <> inline bool +is_a_helper <const gassign *>::test (const_gimple gs) +{ + return gs->code == GIMPLE_ASSIGN; +} + +template <> +template <> +inline bool is_a_helper <gbind *>::test (gimple gs) { return gs->code == GIMPLE_BIND; @@ -2359,8 +2378,8 @@ gimple_assign_set_lhs (gimple gs, tree l static inline tree gimple_assign_rhs1 (const_gimple gs) { - GIMPLE_CHECK (gs, GIMPLE_ASSIGN); - return gimple_op (gs, 1); + const gassign *ass = GIMPLE_CHECK2<const gassign *> (gs); + return ass->op[1]; } Index: gcc/gimple.c =================================================================== --- gcc/gimple.c (revision 226240) +++ gcc/gimple.c (working copy) @@ -89,6 +89,10 @@ static const char * const gimple_alloc_k "everything else" }; +/* Static gimple tuple members. */ +const enum gimple_code gassign::code_; + + /* Gimple tuple constructors. Note: Any constructor taking a ``gimple_seq'' as a parameter, can be passed a NULL to start with an empty sequence. */
I noticed that the code we generate for a simple gimple_assign_rhs1 (stmt) is quite bad as we have two checking pieces. The implementation is now static inline tree gimple_assign_rhs1 (const_gimple gs) { GIMPLE_CHECK (gs, GIMPLE_ASSIGN); return gimple_op (gs, 1); } and the hidden checking is due to gimple_op being static inline tree * gimple_ops (gimple gs) { size_t off; /* All the tuples have their operand vector at the very bottom of the structure. Note that those structures that do not have an operand vector have a zero offset. */ off = gimple_ops_offset_[gimple_statement_structure (gs)]; gcc_gimple_checking_assert (off != 0); return (tree *) ((char *) gs + off); (ugly in itself considering that we just verified we have a gassign which has an operand vector as member...). gimple_op incurs two additional memory reference to compute gimple_statement_structure and gimple_ops_offset_ plus the checking bits. The simplest thing would be to use GIMPLE_CHECK (gs, GIMPLE_ASSIGN); return as_a <const gassign *> (gs)->op[1]; but that's not optimal either because we check we have a gassign again (not optimized in stage1). So I decided to invent a fancy as_a which reports errors similar to GIMPLE_CHECK and makes the code look like const gassign *ass = GIMPLE_CHECK2<const gassign *> (gs); return ass->op[1]; for some reason I needed to overload is_a_helper for const_gimple (I thought the is_a machinery would transparently support qualified types?). I'm missing a fallback for !ENABLE_GIMPLE_CHECKING as well as an overload for 'gimple' I guess. I just changed gimple_assign_rhs1 for now. So this is a RFC. I understand in the end the whole GCC may use gassign everywhere we access gimple_assign_rhs1 but I don't see this happening soon and this simple proof-of-concept patch already reduces unoptimized text size of gimple-match.c from 836080 to 836359 (too bad) and optimized from 585731 to 193190 bytes (yay!) on x86_64 using trunk. Some inlining must go very differently - we just have 544 calls to gimple_assign_rhs1 in gimple-match.c. .optimizes shows all calls remaining as <bb 89>: _ZL18gimple_assign_rhs1PK21gimple_statement_base.part.32 (def_stmt_47); which is tree_node* gimple_assign_rhs1(const_gimple) (const struct gimple_statement_base * gs) { <bb 2>: gimple_check_failed (gs_1(D), &"/space/rguenther/tramp3d/trunk/gcc/gimple.h"[0], 2381, &"gimple_assign_rhs1"[0], 6, 0); } so eventually we just do a better job with profile estimation. I think inliner-wise we are even as we get rid of the gimple_op () inline but instead get the GIMPLE_CHECK2 inline. So - any comments? Thanks, Richard. 2015-07-27 Richard Biener <rguenther@suse.de> * gimple.h (GIMPLE_CHECK2): New inline template function. (gassign::code_): New constant static member. (is_a_helper<const gassign *>): Add. (gimple_assign_rhs1): Use GIMPLE_CHECK2 and a cheaper way to access operand 1. * gimple.c (gassign::code_): Define.