This is the mail archive of the 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: OMP_ATOMIC expand/gimplify changes

On 10/14/07, Razya Ladelsky <> wrote:

> +static bool
> +expand_omp_atomic_fetch_op (basic_block load_bb,
> +		 		 		     tree addr, tree loaded_val,
> +		 		 		     tree stored_val, int index)

Mind formatting here.

> +{
> +  enum built_in_function base;
> +  tree decl, itype, call;
> +  enum insn_code *optab;
> +  tree rhs;
> +  basic_block store_bb = single_succ (load_bb);
> +  block_stmt_iterator bsi;
> +  tree stmt;
> +
> +  /* There should be just one statement computing the stored value.  */

Please describe what is the exact shape of statements that we are
expecting to find in order to expand into an atomic fetch operation.

> +  bsi = bsi_after_labels (store_bb);
> +  stmt = bsi_stmt (bsi);
> +  if (TREE_CODE (stmt) != GIMPLE_MODIFY_STMT)
> +    return false;
> +  bsi_next (&bsi);
> +  if (TREE_CODE (bsi_stmt (bsi)) != OMP_ATOMIC_STORE)
> +    return false;
> +
> +  if (!operand_equal_p (GIMPLE_STMT_OPERAND (stmt, 0),
> +		 		 		 stored_val, 0))
> +    return false;

Instead of this blind structural check of statements, perhaps it would
better to use data flow to pick the statements?  This is not a problem
today, but in the future it may happen that we have code motion passes
before OMP expansion.  In which case, extraneous code may have been
inserted in the middle of this sequence.  This would make us give up
too soon, even if we *could* reconstruct the statements to emit a
proper atomic fetch.

I don't think this is terribly important for now, but we should think
about a better expansion strategy for the future.  Any ideas?  For
now, we could just add a FIXME here for future reference.

> +/* A subroutine of expand_omp_atomic.  Implement the atomic operation as:
> +
> +		 		  oldval = *addr;
> +      repeat:
> +		 		  newval = rhs;		 		  // with oldval replacing *addr in rhs
> +		 		  oldval = __sync_val_compare_and_swap (addr, oldval, newval);
> +		 		  if (oldval != newval)
> +		 		    goto repeat;
> +

Mind formatting here.  Is your mailer inserting tabs or are these
lines really wrapping around?

> +  /* Load the initial value, replacing the OMP_ATOMIC_LOAD.  */
> +  bsi = bsi_last (load_bb);
> +  gcc_assert (TREE_CODE (bsi_stmt (bsi)) == OMP_ATOMIC_LOAD);
> +  initial = force_gimple_operand_bsi (&bsi, build_fold_indirect_ref (addr),
> +		 		 		 		       true, NULL_TREE,
> +		 		 		 		       true, BSI_SAME_STMT);

Same here.  In fact, I see this in several places throughout the
patch.  Have you configured your editor to expand physical tabs to 2
spaces?  If so, that may be the problem.  It's better to have physical
tabs expand at 8 and use soft-tab for indenting.

> +
> +static bool
> +expand_omp_atomic_mutex (basic_block load_bb, basic_block store_bb,
> +		 		 		  tree addr, tree loaded_val, tree stored_val)

Missing documentation for LOADED_VAL and STORED_VAL.  Describe how
they are expanded.

> +/* Gimplify an OMP_ATOMIC statement.  */

s/Gimplify/Expand/.  This needs a high-level description of the three
expansion strategies that are attempted.  REGION should be documented
as well.

> +/* Codes used for lowering of OMP_ATOMIC.  Although the form of the OMP_ATOMIC
> +   statement is very simple (just in form mem op= expr), various implicit
> +   conversions may cause the expression become more complex, so that it does
> +   not fit the gimple grammar very well.  To overcome this problem, OMP_ATOMIC
> +   is rewritten as a sequence of two codes in gimplification:
> +
> +   OMP_LOAD (tmp, mem)
> +   val = some computations involving tmp;
> +   OMP_STORE (val)  */

This needs a better description.  How is VAL related to MEM in the
expansion?  A simple example and a brief description or how we try to
expand the construct would also help.

> +get_addr_dereference_operands (tree stmt, tree *addr, int flags,
> +                                                      tree full_ref,
> +                                                       HOST_WIDE_INT offset, HOST_WIDE_INT size,
> +                                                       bool recurse_on_base)

Why was this needed?  It's not clear to me why you extracted this code
out of get_indirect_ref_operands.

The rest looks fine to me.

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