Project

General

Profile

Bug #166

Bugs in gmx_cyclecounter.h, gmx_fft_fftw3.c and pull.c

Added by Marc Baaden about 12 years ago. Updated almost 12 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Erik Lindahl
Category:
mdrun
Target version:
Affected version - extra info:
Affected version:
Difficulty:
uncategorized
Close

Description

By adapting working code from gmx 3.3 to the latest CVS version (of 28/9/2007)
we stumbled upon a number of errors in the code that were revealed by the
more "selective" compiler on the AIX platform.

Attached are the concerned files and our workarounds. With out modifications
the code compiles, but we were not sure that the results are scientifically correct.
In any case the current lines in the CVS code looked clearly wrong or ambiguous.

The *.ORIG files are the one from CVS, the *.MDD files are our modifications.

In pull.c there is either a pointer comparison problem or the wrong variable is used.
In gmx_fft_ftw3.c is a construct that's not recognized by the AIX compiler
In gmx_cyclecounter.h one variable name is declared, but another used (in an AIX specific routine)

bugs.tar (130 KB) bugs.tar tar file with original and corrected source code Marc Baaden, 09/28/2007 09:33 PM

History

#1 Updated by Marc Baaden about 12 years ago

Created an attachment (id=245)
tar file with original and corrected source code

#2 Updated by Berk Hess about 12 years ago

I fixed the pull.c problems.
Erik should look at the cyclecounter and fftw3 stuff.

Berk.

#3 Updated by Erik Lindahl about 12 years ago

Hi,

The cyclecounter stuff was a simple typo (fixed), but not the FFTW3 code.

The problem is that FFTW3 has separate routines depending on whether the input/output arrays are aligned or not. To set up code for both alternatives we generate an aligned pointer, and then an unaligned by adding 8 bytes. Your change is adding 8 to a real pointer (size 4 or 8), which still keeps it aligned.

I guess the IBM compiler is complaining about pointer arithmetic on void pointers, right?

What happens if you use (char *) instead? That's not guaranteed to be a single byte, but in practice I've never seen any implementation where it isn't...

Alternatively, we could try to cast the pointer over to a size_t, do the arithmetic, and then cast it back.

Cheers,

Erik

#4 Updated by Marc Baaden about 12 years ago

(In reply to comment #3)

Hi,

The cyclecounter stuff was a simple typo (fixed), but not the FFTW3 code.

The problem is that FFTW3 has separate routines depending on whether the
input/output arrays are aligned or not. To set up code for both alternatives we
generate an aligned pointer, and then an unaligned by adding 8 bytes. Your
change is adding 8 to a real pointer (size 4 or 8), which still keeps it
aligned.

I guess the IBM compiler is complaining about pointer arithmetic on void
pointers, right?

Yes, that's right.

What happens if you use (char *) instead? That's not guaranteed to be a single
byte, but in practice I've never seen any implementation where it isn't...

Indeed, that works. And actually - now that you mention it - that's already the case
in other routines, like gmx_fft_init_1d, gmx_fft_init_3d,..
So using that same procedure should be fine.

Alternatively, we could try to cast the pointer over to a size_t, do the
arithmetic, and then cast it back.

Doesn't seem necessary.

Thanks for your assistance,
Marc

NB: I'll leave the bug as assigned for now, as I guess you'll close it when checking in a corrected version in CVS

#5 Updated by Erik Lindahl about 12 years ago

Hi Marc,

Two things:

1. Could you try casting it to size_t, do the arithmetic, and cast back?
The reason for this is that standard C compilers (and optimizers) assume "no aliasing", i.e. that we never have two different pointers referring to the same address. A few compilers tend to complain loud when they still see it.

2. Is it correct that you are using Power6? Then I'd like to test the Altivec/VMX functionality and see if it improves performance :-)
The first test would be to just run ./configure and see what it says in src/config.h - my guess is that we don't enable it by default. After that, try --enable-altivec and see what happens.

Other things that would help me is the output of these commands:

uname -a
/usr/sbin/lsdev -C -c processor -S available
/usr/sbin/lsattr -EHl sys0

#6 Updated by Marc Baaden about 12 years ago

(In reply to comment #5)

Hi Marc,
Two things:
1. Could you try casting it to size_t, do the arithmetic, and cast back?
The reason for this is that standard C compilers (and optimizers) assume "no
aliasing", i.e. that we never have two different pointers referring to the
same address. A few compilers tend to complain loud when they still see it.

Ok, I'll check this.

2. Is it correct that you are using Power6?

Unfortunately not (yet). The current installation at IDRIS where I am testing this is Power4 and Power4+. They might get a Power6 upgrade some time next year (my guess), but nothing official as yet.
However I'll check with my colleage whether we might have access to Power6 within the DEISA architecture.

Then I'd like to test the
Altivec/VMX functionality and see if it improves performance :-)
The first test would be to just run ./configure and see what it says in
src/config.h - my guess is that we don't enable it by default. After that, try
--enable-altivec and see what happens.

Other things that would help me is the output of these commands:

uname -a
/usr/sbin/lsdev -C -c processor -S available
/usr/sbin/lsattr -EHl sys0

#7 Updated by Marc Baaden about 12 years ago

Hi Erik,

I have discussed this with my colleague Gilles Grasseau at Idris who is working on our code and on the IBM architecture. Here are his replies:

1. Could you try casting it to size_t, do the arithmetic, and cast back?
The reason for this is that standard C compilers (and optimizers) assume "no
aliasing", i.e. that we never have two different pointers referring to the
same address. A few compilers tend to complain loud when they still see it.

This is the modification you suggest:
size_t pc;

...
// pc = (void *)p1;
pc = (size_t ) p1;
pc += 8;
up1 = (void *) pc;

It works with the case with we study.

But on some Supercomputer like NEC Super-UX this could fail
because size_t is st by default to 32 bits and adresses are
on 64 bits. A specific option was added to set the size_t on
64 bits.

2. Is it correct that you are using Power6? Then I'd like to test the
Altivec/VMX functionality and see if it improves performance :-)
The first test would be to just run ./configure and see what it says in
src/config.h - my guess is that we don't enable it by default. After that, try
- --enable-altivec and see what happens.

Currently, I have no access to a Power6. I will try to get one but not now
beacause automn is the report season.

(In reply to comment #5)

Hi Marc,

Two things:
[..]

#8 Updated by Erik Lindahl almost 12 years ago

Hi,

I've now switched the calculation to size_t, but I also added a new check in the configure stage to make sure size_t can hold pointer data types.

Also available in: Atom PDF