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]

Re: [PATCH][RFC] Re-work GIMPLE checking to be gimple class friendly


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.  */
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]