Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#8296 closed feature request (fixed)

Patch: new primops for byte range copies ByteArray# <-> Addr#

Reported by: duncan Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.6.3
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

Currently we have:

  • copyByteArray# for ByteArray# to MutableByteArray#
  • copyMutableByteArray# for MutableByteArray# to MutableByteArray#

This patch adds primops for similar cases involving Addr#s, that is pointers to pinned or foreign memory.

  • copyByteArrayToAddr# for ByteArray# to Addr#
  • copyMutableByteArrayToAddr# for MutableByteArray# to Addr#
  • copyAddrToByteArray# for Addr# to MutableByteArray#

These are not covered by the existing primops of course, and are useful because ByteArray#s are a bit special, sometimes being unpinned, and being aligned, and being so primitve/built-in. It's true we could use FFI import of memcpy using the GHC's FFI extension to turn ByteArray# into a pointer on the C side. However we don't do that for the existing primops and we're following the same pattern here. (Good reasons for them all to be primops: abstracts the backend/platform better, can take advantage of known allignment and size info).

In particular, these primops would be useful in the impl of low level libs like bytestring, text, array, binary etc.

Attachments (3)

0001-Add-tests-for-the-new-ByteArray-Addr-copy-primops.patch (7.1 KB) - added by duncan 7 months ago.
Test for new primops
0001-New-primops-for-byte-range-copies-ByteArray-Addr.patch (6.3 KB) - added by duncan 7 months ago.
Patch to add primops, update #1
0001-Fix-the-types-in-the-copy-ByteArray-Addr-tests.patch (2.9 KB) - added by duncan 7 months ago.
Fix to the types in the primops tests

Download all attachments as: .zip

Change History (11)

Changed 7 months ago by duncan

Test for new primops

comment:1 Changed 7 months ago by duncan

BTW, while I've compiled ghc with these and run the individual test, I've not done a full validate run.

comment:2 Changed 7 months ago by duncan

  • Status changed from new to patch

comment:3 Changed 7 months ago by rwbarton

It looks like the comments in doCopyByteArrayToAddrOp and doCopyAddrToByteArrayOp were copied from doCopyByteArrayOp. Actually, the reason it's okay to assume the memory ranges aren't overlapping in the former two functions is that that is a precondition of the primop.

comment:4 follow-up: Changed 7 months ago by thoughtpolice

This patch looks quite straightforward to me. If nobody has any other objections, I'll put it on my patch queue and merge it later tonight.

Changed 7 months ago by duncan

Patch to add primops, update #1

comment:5 in reply to: ↑ 4 Changed 7 months ago by duncan

Replying to rwbarton:

It looks like the comments in doCopyByteArrayToAddrOp and doCopyAddrToByteArrayOp were copied from doCopyByteArrayOp. Actually, the reason it's okay to assume the memory ranges aren't overlapping in the former two functions is that that is a precondition of the primop.

Fair point. Updated those two comments.

Replying to thoughtpolice:

This patch looks quite straightforward to me. If nobody has any other objections, I'll put it on my patch queue and merge it later tonight.

Ta!

comment:6 Changed 7 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Merged.

commit f11289f65e77b9e3178b08f5ca4472762c77c42e
Author: Duncan Coutts <duncan@well-typed.com>
Date:   Fri Sep 13 09:19:24 2013 +0100

    New primops for byte range copies ByteArray# <-> Addr#
    
    We have primops for copying ranges of bytes between ByteArray#s:
     * ByteArray# -> MutableByteArray#
     * MutableByteArray# -> MutableByteArray#
    This extends it with three further cases:
     * Addr# -> MutableByteArray#
     * ByteArray# -> Addr#
     * MutableByteArray# -> Addr#
    One use case for these is copying between ForeignPtr-based
    representations and in-heap arrays (like Text, UArray etc).
    
    The implementation is essentially the same as for the existing
    primops, and shares the memcpy stuff in the code generators.
    
    Defficiencies / future directions: none of these primops (existing
    or the new ones) let one take advantage of knowing that ByteArray#s
    are word-aligned in memory. Though it is unclear that any of the
    code generators would make use of this information unless the size
    to copy is also known at compile time.
    
    Signed-off-by: Austin Seipp <austin@well-typed.com>

Duncan, I also had to fix the actual primops in bb532682aa47c30dfd49039c5ab282352d38dac0, since they claimed to work over ST RealWorld, but instead should work over ST s

commit bb532682aa47c30dfd49039c5ab282352d38dac0
Author: Austin Seipp <austin@well-typed.com>
Date:   Sun Sep 15 15:15:17 2013 -0500

    Fix the type signatures of new copy primops.
    
    They claimed to work over 'ST RealWorld', when instead they should be
    parameterized in the state type. This fixes the cgrun070.
    
    Signed-off-by: Austin Seipp <austin@well-typed.com>

comment:7 Changed 7 months ago by duncan

Oh sorry about that. I see that I changed the primops to use State# RealWorld after writing the tests and then didn't update the tests.

So actually I think using RealWorld here is probably right, because these ops work with Addr#s, both reading and writing from them, and so this does not really make sense in a pure ST context. Admititly, the pattern in the existing primops is not totally clear about this, things like touch# use State# RealWorld, but then all the primops like readIntOffAddr# use State# s.

So not totally clear cut either way. I'll leave the choice to you. If you want to switch back to using State# RealWorld as in my original patch, then I attach the corresponding fix to the tests.

Changed 7 months ago by duncan

Fix to the types in the primops tests

comment:8 Changed 7 months ago by duncan

On reflection, there's probably no harm in leaving the primops as State# s. It's vaguely plausible it could be useful in a context like repa/dph which use Addr#s pointing into pinned ByteArray#s.

Note: See TracTickets for help on using tickets.