User account creation filtered due to spam.

Bug 36886 - misaligment for cshift of character
Summary: misaligment for cshift of character
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libfortran (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: ---
Assignee: Thomas Koenig
URL:
Keywords: wrong-code
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2008-07-21 10:27 UTC by Thomas Koenig
Modified: 2008-08-16 16:37 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2008-07-22 11:58:11


Attachments
proposed patch for cshift0 (3.56 KB, patch)
2008-08-02 23:43 UTC, Thomas Koenig
Details | Diff
better patch (3.56 KB, patch)
2008-08-02 23:49 UTC, Thomas Koenig
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Koenig 2008-07-21 10:27:33 UTC
array->data and ret->data are not
divisible by four in this test case, yet
we use ints for copying.

Not too bad on a i686, very bad on systems that
enforce alignment.

$ cat cshift-char.f90
program main
  character(len=2) :: c2
  character(len=4), dimension(2,2) :: a, b, c, d
  ! Force misalignment of a or b
  common /foo/ a, c, c2, b, d
  a = 'aa'
  b = 'bb'
  d = cshift(b,1)
  c = cshift(a,1)
end program main
$ gfortran -g cshift-char.f90
$ gdb ./a.out
GNU gdb 6.8-debian
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i486-linux-gnu"...
(gdb) b _gfortran_cshift0_4_char
Function "_gfortran_cshift0_4_char" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (_gfortran_cshift0_4_char) pending.
(gdb) r
Starting program: /home/ig25/Krempel/Char/a.out

Breakpoint 1, *_gfortran_cshift0_4_char (ret=0xbfc41720, ret_length=4,
    array=0xbfc41744, pshift=0x8048904, pdim=0x0, array_length=4)
    at ../../../../gcc/trunk/libgfortran/intrinsics/cshift0.c:364
364     DEFINE_CSHIFT (4);
(gdb) p ret->data
$1 = 0x8049ad2 ""
(gdb) p array->data
$2 = 0x8049ac2 "bb  bb  bb  bb  "
(gdb)
Comment 1 Thomas Koenig 2008-07-22 11:58:11 UTC
Related to PR 32972 (optimizing cshift the way that other
array intrinsics have been done will cure this).
Comment 2 Thomas Koenig 2008-08-02 23:43:54 UTC
Created attachment 16000 [details]
proposed patch for cshift0

Here's something that works for cshift0.

cshift1 still to do...
Comment 3 Thomas Koenig 2008-08-02 23:49:38 UTC
Created attachment 16001 [details]
better patch

This one is better :-)
Comment 4 Tobias Burnus 2008-08-03 07:23:31 UTC
> Created an attachment (id=16001) [edit]
> better patch

I think some of the run-time checks (with -fbounds-checks) for PR 36874 can be best done in libgfortran. How about including in your patch a check that the shape of ARRAY (minus dimension DIM) is conformable with the SHIFT argument? (i.e. that for every dimension SHIFT has the same number of elements as ARRAY.)

I was additionally wondering whether one should change error code such as:
  if (which < 0 || (which + 1) > GFC_DESCRIPTOR_RANK (array))
into
  if (__builtin_expect (which < 0 || (which + 1) > GFC_DESCRIPTOR_RANK (array),
      0))
to speed up the run time by a tiny bit.
Comment 5 Thomas Koenig 2008-08-03 09:22:26 UTC
(In reply to comment #4)
> > Created an attachment (id=16001) [edit]
> > better patch
> 
> I think some of the run-time checks (with -fbounds-checks) for PR 36874 can be
> best done in libgfortran. How about including in your patch a check that the
> shape of ARRAY (minus dimension DIM) is conformable with the SHIFT argument?

I've done so for other intrinsics (see PR 34670), and I plan to do so
for cshift as well.

Not with this patch, though; I like to do one thing at a time :-)

> I was additionally wondering whether one should change error code such as:
>   if (which < 0 || (which + 1) > GFC_DESCRIPTOR_RANK (array))
> into
>   if (__builtin_expect (which < 0 || (which + 1) > GFC_DESCRIPTOR_RANK (array),
>       0))
> to speed up the run time by a tiny bit.

Sure, we could do that for all of the run-time checks.
Comment 6 Thomas Koenig 2008-08-14 18:32:54 UTC
Subject: Bug 36886

Author: tkoenig
Date: Thu Aug 14 18:31:32 2008
New Revision: 139111

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=139111
Log:
2008-08-14  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR libfortran/36886
	* Makefile.am:  Added $(i_cshift0_c).
	Added $(i_cshift0_c) to gfor_built_specific_src.
	Add rule to build from cshift0.m4.
	* Makefile.in:  Regenerated.
	* libgfortran.h:  Addedd prototypes for cshift0_i1,
	cshift0_i2, cshift0_i4, cshift0_i8, cshift0_i16,
	cshift0_r4, cshift0_r8, cshift0_r10, cshift0_r16,
	cshift0_c4, cshift0_c8, cshift0_c10, cshift0_c16.
	Define Macros GFC_UNALIGNED_C4 and GFC_UNALIGNED_C8.
	* intrinsics/cshift0.c:  Remove helper functions for
	the innter shift loop.
	(cshift0):  Call specific functions depending on type
	of array argument.  Only call specific functions for
	correct alignment for other types.
	* m4/cshift0.m4:  New file.
	* generated/cshift0_i1.c:  New file.
	* generated/cshift0_i2.c:  New file.
	* generated/cshift0_i4.c:  New file.
	* generated/cshift0_i8:.c  New file.
	* generated/cshift0_i16.c:  New file.
	* generated/cshift0_r4.c:  New file.
	* generated/cshift0_r8.c:  New file.
	* generated/cshift0_r10.c:  New file.
	* generated/cshift0_r16.c:  New file.
	* generated/cshift0_c4.c:  New file.
	* generated/cshift0_c8.c:  New file.
	* generated/cshift0_c10.c:  New file.
	* generated/cshift0_c16.c:  New file.

2008-08-14  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR libfortran/36886
	* gfortran.dg/cshift_char_3.f90:  New test case.
	* gfortran.dg/cshift_nan_1.f90:  New test case.


Added:
    trunk/gcc/testsuite/gfortran.dg/char_cshift_3.f90
    trunk/gcc/testsuite/gfortran.dg/cshift_nan_1.f90
    trunk/libgfortran/generated/cshift0_c10.c
    trunk/libgfortran/generated/cshift0_c16.c
    trunk/libgfortran/generated/cshift0_c4.c
    trunk/libgfortran/generated/cshift0_c8.c
    trunk/libgfortran/generated/cshift0_i1.c
    trunk/libgfortran/generated/cshift0_i16.c
    trunk/libgfortran/generated/cshift0_i2.c
    trunk/libgfortran/generated/cshift0_i4.c
    trunk/libgfortran/generated/cshift0_i8.c
    trunk/libgfortran/generated/cshift0_r10.c
    trunk/libgfortran/generated/cshift0_r16.c
    trunk/libgfortran/generated/cshift0_r4.c
    trunk/libgfortran/generated/cshift0_r8.c
    trunk/libgfortran/m4/cshift0.m4
Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/Makefile.am
    trunk/libgfortran/Makefile.in
    trunk/libgfortran/intrinsics/cshift0.c
    trunk/libgfortran/libgfortran.h

Comment 7 Thomas Koenig 2008-08-16 16:37:00 UTC
Fixed on trunk.

Closing.