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]

[RFA:] Fix PR middle-end/24341 regression, sync builtins barf on low MAX_FIXED_MODE_SIZE


See PR middle-end/24341, a "4.1 regression", where for cris-* I get:

FAIL: gcc.c-torture/compile/sync-1.c  -O0  (test for excess errors)
FAIL: gcc.c-torture/compile/sync-1.c  -O1  (test for excess errors)
FAIL: gcc.c-torture/compile/sync-1.c  -O2  (test for excess errors)
FAIL: gcc.c-torture/compile/sync-1.c  -O3 -fomit-frame-pointer  (test for excess errors)
FAIL: gcc.c-torture/compile/sync-1.c  -O3 -g  (test for excess errors)
FAIL: gcc.c-torture/compile/sync-1.c  -Os  (test for excess errors)

The error is an ICE in attempt to move a BLKmode object using
emit_move_insn.  It is builtins.c:expand_builtin_lock_release
that tries to do that for a BLKmode mem, but the bug is the
BLKmode being there in the first place.

Right, the sync patterns are not implemented yet for the CRIS
port, but even if they were, it seems sync_lock_releaseM
patterns would be redundant (just clearing memory, so move insns
will suffice).  Still, even if *those* patterns were there, the
same problem could presumably be observed for the force_reg call
in that same function.  The BLKmode comes from calling
get_builtin_sync_mode for BUILT_IN_LOCK_RELEASE_8.  It in turn
calls mode_for_size but specifies 1 for the limit parameter,
which causes mode_for_size to return BLKmode, because the CRIS
port has SImode as MAX_FIXED_MODE_SIZE (for which mode_for_size
is the only direct user).

Now, some may think that the bug is setting MAX_FIXED_MODE_SIZE
to SImode instead of the default DImode, but MAX_FIXED_MODE_SIZE
is not what the name may make you think. :-)  I.e. it's not the
size limit of singular data that can be handled at all, but the
unit limit for when GCC decides "by itself" to use a singular
move instruction, like for small-size structure initialization
and moves.  See TFM.  There's certainly movdi in the machine
description, but measurements (long ago) showed that it's better
to let GCC do it by register-sized pieces for the CRIS port.  I
guess that would actually hold as well for those other ports
where movdi is always synthesized by smaller moves.

Anyway, it's the program being compiled that explicitly asks for
__sync_fetch_and_add for DImode, not GCC internally, so I think
it should work, as opposed to emitting an error or
sorry-message.  Just changing the limit parameter to no-limit
seems right.  If the target needs to be consulted, it should be
whether there is a movdi (really: movM for the suggested mode),
not checking a port-specific implementation detail.

Bootstrapped and checked on {powerpc,i686,x86_86}-*-linux-gnu
and besides cris-elf and cris-axis-linux-gnu also cross-tested
for {sh,mips}-elf for good measure.

Ok to commit?

:ADDPATCH sync builtins, middle-end:

	* builtins.c (get_builtin_sync_mode): Make unlimited
	mode_for_size request.

Index: builtins.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/builtins.c,v
retrieving revision 1.479
diff -p -u -u -p -r1.479 builtins.c
--- builtins.c	4 Oct 2005 16:14:52 -0000	1.479
+++ builtins.c	14 Oct 2005 19:32:18 -0000
@@ -5399,7 +5399,9 @@ expand_builtin_fork_or_exec (tree fn, tr
 static inline enum machine_mode
 get_builtin_sync_mode (int fcode_diff)
 {
-  return mode_for_size (BITS_PER_UNIT << fcode_diff, MODE_INT, 1);
+  /* The size is not negotiable, so ask not to get BLKmode in return
+     if the target indicates that a smaller size would be better.  */
+  return mode_for_size (BITS_PER_UNIT << fcode_diff, MODE_INT, 0);
 }
 
 /* Expand the __sync_xxx_and_fetch and __sync_fetch_and_xxx intrinsics.

brgds, H-P


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