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 <rguent...@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. */
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
