Thu Mar 15 12:41:00 GMT 2007
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
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);
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]   ), 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
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?
More information about the Gcc-patches