Opened 9 years ago

Closed 9 years ago

#5210 closed feature request (fixed)

Add primops for copying/cloning byte arrays

Reported by: tibbe Owned by: dterei
Priority: high Milestone: 7.2.1
Component: Compiler Version: 7.0.3
Keywords: Cc: johan.tibell@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Add primops for copying/cloning byte arrays so that we can unroll such copies in the backends. Even if we don't unroll in the backend, this will allow us to use memcpy for copying parts of the byte array, which is difficult today as while ByteArray# can be passed in an FFI call, the pointer that gets passed refers to the based of the array when we might want it to refer to somewhere in the middle of the array.

Change History (20)

comment:1 Changed 9 years ago by tibbe

Status: newpatch
Type: bugfeature request

The attached patches add two new primops: copyByteArray# and copyMutableByteArray#. I didn't any primops corresponding to the clone primops we have for Array# and MutableArray#. Since newByteArray# and its cousins don't initialize the newly created array it should be efficient enough to implement clone as a call to newByteArray#, followed by a call to copyByteArray#.

comment:2 Changed 9 years ago by simonmar

Milestone: 7.2.1
Owner: changed from tibbe to simonmar
Priority: normalhigh

Thanks, I'll review.

comment:3 Changed 9 years ago by rl

It would probably make sense to have this for Addr#, too.

comment:4 Changed 9 years ago by tibbe

Do we have a way to convert an Addr# to a pointer in Haskell code? If so, couldn't one just call memcpy directly on the pointer? The reason I'm adding these primops is that pointer arithmetic is not possible on ByteArray#s, as any interior pointer you grab could be invalid by the time you call memcpy, as the GC might move the array. Since Addr# point outside the heap it should be safe to use pointer arithmetic on them.

In any case, please open a separate ticket.

comment:5 Changed 9 years ago by rl

Addr# is what Ptr and friends are implemented on top of. The operations would only be useful for unrolling, not for the pointer arithmetic.

comment:6 in reply to:  5 Changed 9 years ago by tibbe

Replying to rl:

Addr# is what Ptr and friends are implemented on top of. The operations would only be useful for unrolling, not for the pointer arithmetic.

Fair enough. Once we have the actual unrolling going in the backends (LLVM will be first) we can look into it.

comment:7 Changed 9 years ago by tibbe

I've updated the patches to apply cleanly to the current HEAD. There will be some merging needed once the patch to unroll memcpy in the native codegen has been applied.

(Please ignore the file with .2 in its name. It got attached accidentally.)

Changed 9 years ago by tibbe

comment:8 Changed 9 years ago by tibbe

Owner: changed from simonmar to dterei

I've merged the patches with the latest HEAD and validated them. Please review.

comment:9 Changed 9 years ago by tibbe

Ported the patch to the new code gen as well.

comment:10 Changed 9 years ago by dterei

I like the way the testing is done with having a guard value at each end. My only concern is this line:

-- Assign the arguments to temporaries so the code generator can
-- calculate liveness for us.

I think its fine, its just I don't really understand it. I was originally going to say why are you doing this assigning stuff as the backends will take care of it and then noticed the comment above. Other code seems to follow this pattern so I assume you just kept with the trend.

I'm pretty tired right now so will wait till tomorrow to do a quick check again before pushing.

comment:11 in reply to:  10 Changed 9 years ago by tibbe

The issue is that src, etc might refer to callee-save registers that get trashed by the call to memcpy. The code generator can't figure out that it needs to push/pop these registers before calling the C function so it has to be done manually (this is what the live variable is about). There is a way to have the code generator do it for you, by assigning src, etc to temporary registers, which is what we do. The new code generator will eventually make this unnecessary.

comment:12 Changed 9 years ago by dterei

No I understand the general idea going on in the backend with live vars and all, I'm just not completely convinced this is the way it should be handled. With these patches where you are just making one C function call, src and co aren't really live across the call. Well they might be but the assignTemp stuff to save them is local to our scope so it will only save them for this code where they are only used for one C call so why are we saving them? I'm rusty on this stuff so maybe I'm completely off here. I'll apply the patches now as my only concern is that we may be doing more work, not that the code is wrong.

comment:13 Changed 9 years ago by dterei

Applied:

c7f744bce65cfa2a3d18d9687df889de54333823 b00b36196a88ad6b9054244caaec926f6f9db2cf f141e108deade85c51bf064e9ccbca785ad8e1c1

Do we want to close this ticket now or leave open for tracking future work like the Addr# stuff?

comment:14 in reply to:  12 Changed 9 years ago by tibbe

David you're absolutely correct: we don't need to assign these arguments to temporaries. Fixed by the attached patch:

commit 3473e040abe5a2533078f418646248ec9aeaf750
Author: Johan Tibell <johan.tibell@gmail.com>
Date:   Fri Jun 17 11:13:37 2011 +0200

    codeGen: Make emitCopyByteArray less pessimistic
    
    Assigning the arguments to temporaries was only needed in the case of
    emitCopyArray, where the arguments are alive across the call.  That is
    not the case in emitCopyByteArray.

comment:15 Changed 9 years ago by tibbe

I won't have time to add a Addr# version before 7.2. If someone else has the time feel free to add it.

comment:16 Changed 9 years ago by dterei

Resolution: fixed
Status: patchclosed

OK applied: 530e25a6a1aaccc14630842f310bb4194ce97be0

Closing ticket, we can reopen a different one for Addr# stuff if we decide to pursue.

Note: See TracTickets for help on using tickets.