Re: Thread-safe profiling

[Michael Matz <matz at suse dot de>]

Hi,

On Wed, 14 Mar 2007, Jan Hubicka wrote:

> > On the course of implementing this I hit some bugs in the expansion 
> > code of these builtins and the i386 backend, which are also fixed by 
> > this patch.
> 
> This would be probably worth backporting separately to release 
> branch(es).

Hmm, actually I have a problem with this part now, in that I get ICEs now 
in some circumstances, not in the i386 backend, but due to my other fixes 
in builtins.c, namely these (and the similar) changes:

@@ -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 :-(

Namely for such call:

__sync_fetch_and_add_1 (ptr, -1);

The tree argument 1 will be: <integer_cst <unsigned char> 255>, which is 
correct, as the prototype for this builtin indeed has an unsigned char 
integer type in arguments.  Now the expand_expr above will turn that into 
(const_int 255 [0xff]), also still sort of correct.  With my patch this 
will not be touched by convert_to_mode, so will go into 
expand_sync_operation, and because that constant doesn't fit the predicate 
of the sync_add instruction (it's not a trunc_int_for_mode fixpoint) will 
generate a move to a QImode register, which then finally can't be 
recognized (for the same reason of invalid QImode constant).

_Without_ my patch this (const_int 255) would be put through 
convert_to_mode (QImode, (const_int 255), 1/*unsigned*/), and magically 
come out as (const_int -1), which I find a bit curious given the requested 
unsignedness, but it will at least make the code work, because that 
constant is of course a trunc_int_for_mode fixpoint (meaning that
trunc_int_for_mode (x) == x, which is necessary for nonmemory_operand).

Soo, ignoring that I find all this handling of const_ints a bit peculiar, 
I'm still left with the fact, that if I do the convert_to_mode there the 
DImode cases will be broken, and if I don't do it, the QImode (and 
HImode) cases will be broken.  Curse upon who made const_ints not have 
modes :-/

For testing purposes I also changed the convert_to_mode calls to convert 
as signed (and doing it all the time), which seems to work, but given that 
all quantities involved are actually unsigned this seems even more fishy.  

Can anyone give an advice how this all should behave, and why perhaps 
doing this conversion signed may be indeed the right fix?


Ciao,
Michael.