Thread-safe profiling

Michael Matz matz@suse.de
Thu Mar 15 14:37:00 GMT 2007


Hi,

On Thu, 15 Mar 2007, Jan Hubicka wrote:

> > @@ -5717,8 +5717,10 @@ expand_builtin_sync_operation (enum mach
> >    mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);
> > 
> >    val = expand_expr (CALL_EXPR_ARG (exp, 1), NULL, mode, EXPAND_NORMAL);
> > -  /* If VAL is promoted to a wider mode, convert it back to MODE.  */
> > -  val = convert_to_mode (mode, val, 1);
> > +  /* If VAL is promoted to a wider mode, convert it back to MODE.
> > +     Not for CONST_INTs though.  */
> > +  if (GET_MODE (val) != VOIDmode)
> > +    val = convert_to_mode (mode, val, 1);
> > 
> >    if (ignore)
> >      return expand_sync_operation (mem, val, code);
> > 
> > 
> > Without this change, this call will be miscompiled on 32bit hosts:
> > 
> > __sync_fetch_and_add_8 (ptr, -(unsigned long long)1);
> > 
> > Because the -1LL will be 0xffffffffffffffff in the tree expression.  Now 
> > the expand_expr call above will turn that tree constant into a RTL 
> > constant, namely (const_int -1 [0xffffffff]), that is correct as far as I 
> > can see, as RTL constants are implicitely sign extended.
> > 
> > Now, without my change the convert_to_mode() call will turn that RTL 
> > constant into the wrong 64bit unsigned variant of this (mode is DImode, 
> > and due to the unsigned flag being set for the convert_to_mode call), 
> > namely (const_int [0xffffffff] [0] [0] [0]), i.e. it's zero extended.
> > 
> > So, my thought was to not call convert_to_mode for CONST_INTs at all, 
> > which seems what other code sometimes is doing.  This works fine for the 
> > DImode case, but I'm now seeing e.g. QImode failures :-(
> 
> Hmm, is thre reason why convert_modes doesn't work?

convert_to_mode is just a wrapper around convert_modes.  In the cases at 
hand (the input is a (const_int), i.e. input mode is VOIDmode) it would be 
exactly equivalent to convert_modes,

> (also gen_int_mode will get you appropriate constants)

I don't have directly constant inputs.  It's a general expander of 
the builtin function, so it has to work with pseudos, constants, 
expressions, anything.  For reference here the code again (before my 
patch):

static rtx
expand_builtin_sync_operation (enum machine_mode mode, tree exp,
                               enum rtx_code code, bool after,
                               rtx target, bool ignore)
{
  rtx val, mem;

  /* Expand the operands.  */
  mem = get_builtin_sync_mem (CALL_EXPR_ARG (exp, 0), mode);

  val = expand_expr (CALL_EXPR_ARG (exp, 1), NULL, mode, EXPAND_NORMAL);
  /* If VAL is promoted to a wider mode, convert it back to MODE.  */
  val = convert_to_mode (mode, val, 1);
  ...

'exp' is a general CALL_EXPR, and argument 1 can be any valid tree.  I 
don't see how gen_int_mode would help me here.  Sure I could hack around, 
and test val for being CONST_INT, and then generate an own RTL constant by 
hand again, instead of going over convert_to_mode(), but that can't be the 
solution.

The more I think about it, the more I think const_int's are broken 
somewhere.


Ciao,
Michael.



More information about the Gcc-patches mailing list