[armel tizen] Implemented WriteBarriers.S stubs #4235
Conversation
|
@Dmitri-Botcharnikov @BredPet @jkotas |
| // r1 = value | ||
| LEAF_ENTRY RhpCheckedXchg, _TEXT | ||
| ALTERNATE_ENTRY RhpCheckedXchgAVLocation | ||
| ldr r2, [r0] |
There was a problem hiding this comment.
This needs to be atomic operation - like that what it is in https://github.com/dotnet/corert/blob/master/src/Native/Runtime/arm/WriteBarriers.asm#L403
| // r1 = value | ||
| // r2 = comparand | ||
| LEAF_ENTRY RhpCheckedLockCmpXchg, _TEXT | ||
| PROLOG_PUSH "{r4,lr}" |
There was a problem hiding this comment.
This does not need scratch registers - it should be like the Windows version: https://github.com/dotnet/corert/blob/master/src/Native/Runtime/arm/WriteBarriers.asm#L343
|
Could you please cross check your implementation with the Windows ARM implementation? The write barrier implementation should be pretty similar. |
|
@jkotas please review again. Thanks for your notes. |
| @@ -1,68 +1,378 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // track this write. The location address is translated into an offset in the card table bitmap. We set | ||
| // an entire byte in the card table since it's quicker than messing around with bitmasks and we only write | ||
| // the byte if it hasn't already been done since writes are expensive and impact scaling. | ||
| lsr r2, #LOG2_CLUMP_SIZE |
There was a problem hiding this comment.
This shift should be done as part of address mode - look for add r12, r12, $DESTREG, lsr #10 in the Windows version
| // track this write. The location address is translated into an offset in the card table bitmap. We set | ||
| // an entire byte in the card table since it's quicker than messing around with bitmasks and we only write | ||
| // the byte if it hasn't already been done since writes are expensive and impact scaling. | ||
| lsr r0, #LOG2_CLUMP_SIZE |
There was a problem hiding this comment.
This shift should be done as part of address mode - look for add r12, r12, $DESTREG, lsr #10 in the Windows version
|
|
||
| // Export the canonical write barrier under unqualified name as well | ||
| .ifc \REFREG, r1 | ||
| ALTERNATE_ENTRY RhpAssignRef | ||
| ALTERNATE_ENTRY RhpAssignRefAVLocation |
There was a problem hiding this comment.
The AV location needs to point to instruction that will AV, not dmb - to match https://github.com/dotnet/corert/blob/master/src/Native/Runtime/arm/WriteBarriers.asm#L307
| // location is in one of the other general registers determined by the value of REFREG. | ||
|
|
||
| // WARNING: Code in EHHelpers.cpp makes assumptions about write barrier code, in particular: | ||
| // - Function "InWriteBarrierHelper" assumes an AV due to passed in null pointer will happen on the first instruction |
There was a problem hiding this comment.
Once the other issue is fixed, this comment should be updated accordingly - to match https://github.com/dotnet/corert/blob/master/src/Native/Runtime/arm/WriteBarriers.asm#L276
| ldr r12, =C_FUNC(g_ephemeral_low) | ||
| ldr r12, [r12] | ||
| cmp \REFREG, r12 | ||
| blo LOCAL_LABEL(\BASENAME\()_NoBarrierRequired_\REFREG) |
There was a problem hiding this comment.
The target of this conditional branch is unconditional branch. It can be short-circuited to branch directly to LOCAL_LABEL(\BASENAME\()_EXIT_\REFREG)
|
|
||
| // Now check that the real heap location still contains the value we just wrote into the shadow heap. This | ||
| // read must be strongly ordered wrt to the previous write to prevent race conditions. We also need to | ||
| // recover the old value of DESTREG for the comparison so use an xchg instruction (which has an implicit lock |
There was a problem hiding this comment.
xchg is x64-specific instruction. It does not seem to be used here. Should this rather match https://github.com/dotnet/corert/blob/master/src/Native/Runtime/arm/WriteBarriers.asm#L82 ?
|
@jkotas please review once more |
Correctness was checked by comparing with execution of the portable variant on the test Simple/Hello