Don't perform unaligned writes #133

jrtc27 · 2017-07-24T20:55:01Z

Some architectures, such as SPARC, trap on unaligned memory accesses, and poke makes no guarantees about its behaviour if the address is not sufficiently aligned. Therefore we should use a temporary buffer if asked to write to an unaligned address.

bgamari · 2017-07-24T21:12:10Z

Must we always do this? It feel like this should be conditional on the platform.

jrtc27 · 2017-07-24T21:14:53Z

Yes, some platforms will let you get away with it (at a potential performance cost, especially if the kernel has to emulate it, such as on HP PA-RISC). GHC itself has a list of good and bad architectures (bewareLoadStoreAlignment in compiler/cmm/PprC.hs); I don't suppose there's an easy way to get that information?

bgamari · 2017-07-24T21:25:49Z

Indeed some platforms deal quite poorly with unaligned accesses; however, X86, which constitutes the majority of our users, is not one of these. AFAIK the penalty (assuming you don't cross a cache line) is only a cycle or two. However, this patch imposes an additional branch on all writes, aligned or otherwise. Mis-predicted branches are expensive yet sadly GHC doesn't currently give you the ability to ensure that it's predicted correctly. This is why I say we should make this conditional.

GHC itself has a list of good and bad architectures (bewareLoadStoreAlignment in compiler/cmm/PprC.hs); I don't suppose there's an easy way to get that information?

Hmm, sadly I don't there there is. However, maybe this is something that we want to expose; perhaps unalignedAccessesAllowed :: Bool flag in, for instance, GHC.Constants?

Naturally, this wouldn't help you today, but at least it's moving in the right direction. As for today, I think we'll just need to have some #ifdefs, sadly.

jrtc27 · 2017-07-24T21:27:11Z

That might be sensible, but for now perhaps we could just run it through the preprocessor and use a bunch of #if defined(__i386__) || defined(__x86_64__) || defined(__powerpc__) ...?

bgamari · 2017-07-24T22:40:12Z

Yep, I think that is the right answer for now.

jrtc27 · 2017-07-25T20:24:10Z

Updated to make use of the preprocessor. I'm not sure whether foo_HOST_ARCH is the right variable to be checking, as from the point of view of the code being compiled, it cares about the host arch, but if the variable is being set based on what the installed (or just-built) GHC was configured for, then it should really be the target arch, as that's what GHC is producing code for. The foo_TARGET_ARCH macros are supposedly deprecated though according to the header.

bgamari · 2017-07-27T17:36:29Z

Indeed I'm a bit skeptical that HOST_ARCH is correct here. What header in particular are you referring to?

bgamari · 2017-07-27T17:43:07Z

I see, ghcplatform.h. Yes, I'm very skeptical of all of this. At very least it seems like the HOST_ARCH macros are misleadingly named. I'm looking into it

Regarding the CI failure, this is rather confusing and perhaps concerning. I can't reproduce them locally and presumably you can't either (right?). I'll also need to look into this.

jrtc27 · 2017-07-27T18:27:31Z

I can reproduce the issues, but they happen locally with master too. There are two problems:

With QuickCheck 2.10+, the test suite fails to compile, as QuickCheck added its own Arbitrary instance for CChar, but bytestring defines it itself. Commenting bytestring's instance out gives:
Failing tests; I haven't investigated them, but they're not caused by my changes.

bgamari · 2017-07-28T02:34:35Z

Indeed, I have a commit fixing QuickCheck issue. I'm much more concerned about the failing tests. I think we should get to the bottom of this before merging anything else.

bgamari · 2017-08-01T16:42:12Z

Alright, I believe the spurious failures should now be fixed. Sorry for the wait, @jrtc27. Do you think you could rebase this?

jrtc27 · 2017-08-01T17:49:46Z

All tests now passing :)

jrtc27 · 2017-08-16T19:42:40Z

Ping; is this blocked on the host/target macros issue, or mergeable as-is?

bgamari · 2017-08-16T20:19:26Z

We can merge as-is as far as I'm concerned. @dcoutts?

jrtc27 · 2017-09-01T18:08:27Z

Ping :)

dcoutts

Ok. This is ok in that it works, but the extra with allocation is going to be terrible for performance.

BTW, what about ARM platforms?

This is another example of why I want new ghc primops specifically for safe unaligned writes, since the compiler knows the platform and can probably do a better job at generating safe and reasonably good code for unaligned writes. For example some platforms have specific CPU instructions for unaligned reads & writes.

jrtc27 · 2017-09-01T21:59:40Z

Old AArch32 (pre-v6) can do weird things on unaligned accesses (e.g. rotating the byte value by how many bytes you are off from the aligned-down address by), but I believe it works. In theory I think AArch64 can choose not to support unaligned accesses, but in practice I doubt that is enforced (and if the hardware doesn't support it, the kernel likely emulates it). Currently the unregisterised C backend assumes AArch32 and AArch64 cannot perform unaligned accesses, hence why I did not whitelist it here, but since there have been no bug reports about Cabal (which I saw reliably crashing due to this on SPARC with my experimental native code-gen) not working on these platforms, it's probably actually ok. I don't want to make that call and break it though as I'm no expert on Arm :)

jrtc27 · 2017-09-01T22:03:52Z

And yes, this isn't going to be great for performance, but unless you can think of a better solution (while GHC doesn't let you do a pokeUnaligned), it's the price you have to pay for correctness.

jrtc27 · 2017-10-15T14:14:43Z

Ping; is there anything blocking this? Did you want me to whitelist AArch32 and/or AArch64?

bgamari · 2017-10-16T03:43:19Z

James Clarke <notifications@github.com> writes:

Ping; is there anything blocking this? Did you want me to whitelist AArch32 and/or AArch64?

Pinging @dcoutts.

Mistuke · 2017-10-16T08:19:06Z

Currently the unregisterised C backend assumes AArch32 and AArch64 cannot perform unaligned accesses.

yeah and it's wrong, I remember asking why this was made so but.. Anyways, for the ARM targets you can just check for the macro __ARM_FEATURE_UNALIGNED instead. This always returns true for AArch64 and for AArch32 will return the correct value for the given compiler options. It's part of ACLE so both GCC and CLANG and other compliant compilers should define it.

jrtc27 · 2017-10-16T19:54:08Z

Updated; is that what you had in mind?

Mistuke · 2017-10-16T20:32:11Z

Yeah, although (defined(arm_HOST_ARCH) || defined(aarch64_HOST_ARCH)) is not needed since only Arm targets define __ARM_FEATURE_UNALIGNED so the check can be simplified :).

jrtc27 · 2017-10-16T20:33:31Z

Well, you'd hope so, but who knows; I just did that to be extra safe! I can simplify it if you want though.

Mistuke · 2017-10-16T20:40:32Z

I'd say if something goes through the trouble of emulating __ARM_FEATURE_UNALIGNED while not being an ARM target, I'd say it deserves to fail :). Anyway, doesn't really matter, just wanted to mention it.

jrtc27 · 2017-10-20T18:53:57Z

So is everyone happy with this now? If so, could it be merged please? :)

hvr · 2017-10-20T20:40:44Z

@jrtc27 one last thing... would you mind adding a Changelog entry to the 0.10.10 entry? then I'll happily merge this one.

Some architectures, such as SPARC, trap on unaligned memory accesses, and poke makes no guarantees about its behaviour if the address is not sufficiently aligned. Therefore we should use a temporary buffer if asked to write to an unaligned address, unless we know for sure the architecture supports it without trapping.

jrtc27 · 2017-11-06T11:45:12Z

Rebased with a changelog entry (sorry for the delay, I completely forgot about this).

jrtc27 · 2018-03-14T12:22:22Z

Ping

jrtc27 · 2019-01-22T15:53:59Z

Ping? Seems this got forgotten about by everyone involved.

Mistuke · 2019-01-22T19:40:39Z

@hvr anything outstanding here?

chessai · 2019-10-13T04:08:45Z

Ping

sjakobi

Looks like this is good to go. @hvr, @dcoutts could you merge?

Bodigrim

It seems this has already been approved by @dcoutts and signed off by @hvr above. @cartazio could you possibly resolve a conflict in Changelog.md and merge?

Bodigrim · 2020-05-11T19:10:02Z

@jrtc27 could you please rebase?

fumieval · 2020-06-23T22:36:22Z

@Bodigrim I rebased the branch and created a new pull request as #232.

chessai · 2020-06-25T05:03:02Z

Closing in favour of #232

jrtc27 force-pushed the alignment branch from e6f8a45 to 03c4186 Compare July 24, 2017 21:18

jrtc27 force-pushed the alignment branch from 03c4186 to 2bea5ac Compare July 25, 2017 20:18

jrtc27 force-pushed the alignment branch from 2bea5ac to 3eaaf8a Compare August 1, 2017 17:27

dcoutts approved these changes Sep 1, 2017

View reviewed changes

jrtc27 force-pushed the alignment branch from 3eaaf8a to 21aa25d Compare October 16, 2017 19:53

jrtc27 force-pushed the alignment branch from 21aa25d to 990163c Compare November 6, 2017 11:44

sjakobi approved these changes Dec 19, 2019

View reviewed changes

sjakobi mentioned this pull request Dec 19, 2019

Fixed-width hexadecimal encodings perform unaligned writes #184

Closed

Bodigrim approved these changes May 10, 2020

View reviewed changes

vdukhovni mentioned this pull request May 25, 2020

Would be nice to push a 0.10.10.1 release #214

Closed

fumieval mentioned this pull request Jun 23, 2020

Don't perform unaligned writes (REBASED) #232

Merged

chessai closed this Jun 25, 2020

Don't perform unaligned writes #133

Don't perform unaligned writes #133

Conversation

jrtc27 commented Jul 24, 2017

bgamari commented Jul 24, 2017

jrtc27 commented Jul 24, 2017

bgamari commented Jul 24, 2017 • edited Loading

jrtc27 commented Jul 24, 2017

bgamari commented Jul 24, 2017

jrtc27 commented Jul 25, 2017

bgamari commented Jul 27, 2017

bgamari commented Jul 27, 2017

jrtc27 commented Jul 27, 2017

bgamari commented Jul 28, 2017

bgamari commented Aug 1, 2017

jrtc27 commented Aug 1, 2017

jrtc27 commented Aug 16, 2017

bgamari commented Aug 16, 2017

jrtc27 commented Sep 1, 2017

dcoutts left a comment

Choose a reason for hiding this comment

jrtc27 commented Sep 1, 2017 • edited Loading

jrtc27 commented Sep 1, 2017

jrtc27 commented Oct 15, 2017

bgamari commented Oct 16, 2017 via email

Mistuke commented Oct 16, 2017

jrtc27 commented Oct 16, 2017

Mistuke commented Oct 16, 2017

jrtc27 commented Oct 16, 2017

Mistuke commented Oct 16, 2017

jrtc27 commented Oct 20, 2017

hvr commented Oct 20, 2017

jrtc27 commented Nov 6, 2017

jrtc27 commented Mar 14, 2018

jrtc27 commented Jan 22, 2019

Mistuke commented Jan 22, 2019

chessai commented Oct 13, 2019

sjakobi left a comment

Choose a reason for hiding this comment

Bodigrim left a comment

Choose a reason for hiding this comment

Bodigrim commented May 11, 2020

fumieval commented Jun 23, 2020

chessai commented Jun 25, 2020

bgamari commented Jul 24, 2017 •

edited

Loading

jrtc27 commented Sep 1, 2017 •

edited

Loading