Opened 16 years ago

Closed 16 years ago

Last modified 13 months ago

#1175 closed defect (wontfix)

SCUMM_NEED_ALIGNMENT detection bogus

Reported by: SF/mellum Owned by: fingolfin
Priority: normal Component: Ports
Keywords: Cc:
Game:

Description

SCUMM_NEED_ALIGNMENT tries to detect whether unaligned
accesses "work". Unfortunately, this is bogus; many
systems, like Alpha, don't support unaligned accesses,
but catch them with a trap, fix them in kernel space
and continue. Therefore, it will appear they work; only
they slow down the program by a factor of 100. The only
reasonable method is to hardcode architectures where it
is known that unaligned accesses are OK and fast.
Anyway, I doubt it is of much use for code like this:

#if defined(SCUMM_NEED_ALIGNMENT)
memcpy(dst, src, 8);
#else
((uint32 *)dst)[0] = ((const uint32 *)src)[0];
((uint32 *)dst)[1] = ((const uint32 *)src)[1];
#endif

If gcc doesn't generate optimal code for memcpy(dst,
src, 8) for your platform, you should rather file a bug
report against gcc than kludge around it.

Ticket imported from: #791741. Ticket imported from: bugs/1175.

Change History (4)

comment:1 by SF/bbrox, 16 years ago

Well, usually, a configure script should not check for
specific platforms but to see if the platform supports a
given feature or not (otherwise, each time someone tries on
a platform we have no knowledge of, we would need to change
the configure script). So, basically, the configure test is
sound as ScummVM will work on this platform as unaligned
memory accesses are supported (emulated, true, but supported).

This check works on the platform we had access to
(Linux/ARM, Solaris/SPARC, MIPS, ...). So the best would be
for you to send a patch adding an Alpha-only override.
Wonder why, though, the system has this trap mechanism
whereas most other archs do not have it.

Now, as for the 'memcpy' thingy, the thing is that the
'#else' part was first and when someone had a crash on a
platform like ARM, he added the 'memcpy' code and let the
other code stay in place. I agree though that it could be
merged :-)

comment:2 by fingolfin, 16 years ago

Owner: set to fingolfin
Resolution: wontfix
Status: newclosed

comment:3 by fingolfin, 16 years ago

We will not change the alignment detection to be arch based, that
would be a very bad idea, as bbrox explained. However, we could
add a special case for the Alpha; it's up to you or another Alpha
user to submit an according patch via the patch tracker.

As for just using memcpy directly: Not everybody is using gcc, or
a compiler which optimizes away memcpy for static len params. I
am not even sure whether GCC really really will generate
equivalent code for memcpy & our "hand optimized" copy code.
But even if it does, it doesn't matter at all: GCC is not the only
compiler we care about.

comment:4 by digitall, 13 months ago

Component: --Unset--Ports
Note: See TracTickets for help on using tickets.