Re: [ros-dev] [ros-diffs] [hbelusca] 64773: [FAST486]: Do not call RtlCopyMemory for copying few bytes (2 and 4).

[Alex Ionescu]

memcpy already has logic to handle, 1, 2, 4, 8, etc.. sizes.

Best regards,
Alex Ionescu

On Thu, Oct 16, 2014 at 2:48 PM, <hbelusca(a)svn.reactos.org> wrote:

>   Author: hbelusca
>  Date: Thu Oct 16 21:48:18 2014
>  New Revision: 64773
>
>  URL: http://svn.reactos.org/svn/reactos?rev=64773&view=rev
>  Log:
>  [FAST486]: Do not call RtlCopyMemory for copying few bytes (2 and 4).
>
>  Modified:
>      trunk/reactos/lib/fast486/fast486.c
>      trunk/reactos/lib/fast486/opgroups.c
>
>  Modified: trunk/reactos/lib/fast486/fast486.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/fast486/fast486.c?rev=…
>
>  ==============================================================================
>  --- trunk/reactos/lib/fast486/fast486.c [iso-8859-1] (original)
>  +++ trunk/reactos/lib/fast486/fast486.c [iso-8859-1] Thu Oct 16 21:48:18
>  2014
>  @@ -117,7 +117,6 @@
>   Fast486MemReadCallback(PFAST486_STATE State, ULONG Address, PVOID Buffer,
>  ULONG Size)
>   {
>       UNREFERENCED_PARAMETER(State);
>  -
>       RtlMoveMemory(Buffer, (PVOID)Address, Size);
>   }
>
>  @@ -126,7 +125,6 @@
>   Fast486MemWriteCallback(PFAST486_STATE State, ULONG Address, PVOID
>  Buffer, ULONG Size)
>   {
>       UNREFERENCED_PARAMETER(State);
>  -
>       RtlMoveMemory((PVOID)Address, Buffer, Size);
>   }
>
>
>  Modified: trunk/reactos/lib/fast486/opgroups.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/fast486/opgroups.c?rev…
>
>  ==============================================================================
>  --- trunk/reactos/lib/fast486/opgroups.c        [iso-8859-1] (original)
>  +++ trunk/reactos/lib/fast486/opgroups.c        [iso-8859-1] Thu Oct 16
>  21:48:18 2014
>  @@ -2016,6 +2016,7 @@
>
>   FAST486_OPCODE_HANDLER(Fast486OpcodeGroup0F01)
>   {
>  +    // FAST486_TABLE_REG TableReg;
>       UCHAR TableReg[6];
>       FAST486_MOD_REG_RM ModRegRm;
>       BOOLEAN OperandSize, AddressSize;
>  @@ -2054,8 +2055,9 @@
>               }
>
>               /* Fill the 6-byte table register */
>  -            RtlCopyMemory(TableReg, &State->Gdtr.Size, sizeof(USHORT));
>  -            RtlCopyMemory(&TableReg[sizeof(USHORT)],
>  &State->Gdtr.Address, sizeof(ULONG));
>  +            // TableReg = State->Gdtr;
>  +            *((PUSHORT)&TableReg) = State->Gdtr.Size;
>  +            *((PULONG)&TableReg[sizeof(USHORT)]) = State->Gdtr.Address;
>
>               /* Store the GDTR */
>               return Fast486WriteMemory(State,
>  @@ -2076,8 +2078,9 @@
>               }
>
>               /* Fill the 6-byte table register */
>  -            RtlCopyMemory(TableReg, &State->Idtr.Size, sizeof(USHORT));
>  -            RtlCopyMemory(&TableReg[sizeof(USHORT)],
>  &State->Idtr.Address, sizeof(ULONG));
>  +            // TableReg = State->Idtr;
>  +            *((PUSHORT)&TableReg) = State->Idtr.Size;
>  +            *((PULONG)&TableReg[sizeof(USHORT)]) = State->Idtr.Address;
>
>               /* Store the IDTR */
>               return Fast486WriteMemory(State,
>  @@ -2117,7 +2120,8 @@
>               }
>
>               /* Load the new GDT */
>  -            State->Gdtr.Size = *((PUSHORT)TableReg);
>  +            // State->Gdtr = TableReg;
>  +            State->Gdtr.Size = *((PUSHORT)&TableReg);
>               State->Gdtr.Address = *((PULONG)&TableReg[sizeof(USHORT)]);
>
>               /* In 16-bit mode the highest byte is masked out */
>  @@ -2156,7 +2160,8 @@
>               }
>
>               /* Load the new IDT */
>  -            State->Idtr.Size = *((PUSHORT)TableReg);
>  +            // State->Idtr = TableReg;
>  +            State->Idtr.Size = *((PUSHORT)&TableReg);
>               State->Idtr.Address = *((PULONG)&TableReg[sizeof(USHORT)]);
>
>               /* In 16-bit mode the highest byte is masked out */
>
>
>

[Love Nystrom]

@Hermes, @Alex,

May I suggest using my LITERAL macro for that sort of thing..?
The explicit leftcast blurb is not exactly best for readability,
and a call overhead for such a puny transfer is bad performance.

#define LITERAL( type,var )  (*(type*)&var)  // Omnipotent cast

Then
     *((PUSHORT)&TableReg) = State->Idtr.Size;
     *((PULONG)&TableReg[sizeof(USHORT)]) = State->Idtr.Address;

Becomes
     LITERAL( USHORT, TableReg ) = State->Idtr.Size;
     LITERAL( ULONG, TableReg[2] ) = State->Idtr.Address;

Which is far easier to read, ergo less prone to mistakes,
and does not incur a call overhead.

Best Regards
// Love

On 2014-10-21 01.50, Alex Ionescu wrote:
>   memcpy already has logic to handle, 1, 2, 4, 8, etc..
> sizes.
>
>  Best regards,
>  Alex Ionescu
>
>  On Thu, Oct 16, 2014 at 2:48 PM, <hbelusca(a)svn.reactos.org 
>  <mailto:[e-mail]>> wrote:
>
>      Author: hbelusca
>      Date: Thu Oct 16 21:48:18 2014
>      New Revision: 64773
>
>      URL: http://svn.reactos.org/svn/reactos?rev=64773&view=rev
>      Log:
>      [FAST486]: Do not call RtlCopyMemory for copying few bytes (2 and 4).
>
>      Modified:
>          trunk/reactos/lib/fast486/fast486.c
>          trunk/reactos/lib/fast486/opgroups.c
>
>      Modified: trunk/reactos/lib/fast486/fast486.c
>      URL:
>     
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/fast486/fast486.c?rev=…
>      ==============================================================================
>      --- trunk/reactos/lib/fast486/fast486.c [iso-8859-1] (original)
>      +++ trunk/reactos/lib/fast486/fast486.c [iso-8859-1] Thu Oct 16
>      21:48:18 2014
>      @@ -117,7 +117,6 @@
>       Fast486MemReadCallback(PFAST486_STATE State, ULONG Address, PVOID
>      Buffer, ULONG Size)
>       {
>           UNREFERENCED_PARAMETER(State);
>      -
>           RtlMoveMemory(Buffer, (PVOID)Address, Size);
>       }
>
>      @@ -126,7 +125,6 @@
>       Fast486MemWriteCallback(PFAST486_STATE State, ULONG Address,
>      PVOID Buffer, ULONG Size)
>       {
>           UNREFERENCED_PARAMETER(State);
>      -
>           RtlMoveMemory((PVOID)Address, Buffer, Size);
>       }
>
>
>      Modified: trunk/reactos/lib/fast486/opgroups.c
>      URL:
>     
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/fast486/opgroups.c?rev…
>      ==============================================================================
>      --- trunk/reactos/lib/fast486/opgroups.c        [iso-8859-1]
>      (original)
>      +++ trunk/reactos/lib/fast486/opgroups.c        [iso-8859-1] Thu
>      Oct 16 21:48:18 2014
>      @@ -2016,6 +2016,7 @@
>
>       FAST486_OPCODE_HANDLER(Fast486OpcodeGroup0F01)
>       {
>      +    // FAST486_TABLE_REG TableReg;
>           UCHAR TableReg[6];
>           FAST486_MOD_REG_RM ModRegRm;
>           BOOLEAN OperandSize, AddressSize;
>      @@ -2054,8 +2055,9 @@
>                   }
>
>                   /* Fill the 6-byte table register */
>      -            RtlCopyMemory(TableReg, &State->Gdtr.Size,
>      sizeof(USHORT));
>      -            RtlCopyMemory(&TableReg[sizeof(USHORT)],
>      &State->Gdtr.Address, sizeof(ULONG));
>      +            // TableReg = State->Gdtr;
>      +            *((PUSHORT)&TableReg) = State->Gdtr.Size;
>      +            *((PULONG)&TableReg[sizeof(USHORT)]) =
>      State->Gdtr.Address;
>
>                   /* Store the GDTR */
>                   return Fast486WriteMemory(State,
>      @@ -2076,8 +2078,9 @@
>                   }
>
>                   /* Fill the 6-byte table register */
>      -            RtlCopyMemory(TableReg, &State->Idtr.Size,
>      sizeof(USHORT));
>      -            RtlCopyMemory(&TableReg[sizeof(USHORT)],
>      &State->Idtr.Address, sizeof(ULONG));
>      +            // TableReg = State->Idtr;
>      +            *((PUSHORT)&TableReg) = State->Idtr.Size;
>      +            *((PULONG)&TableReg[sizeof(USHORT)]) =
>      State->Idtr.Address;
>
>                   /* Store the IDTR */
>                   return Fast486WriteMemory(State,
>      @@ -2117,7 +2120,8 @@
>                   }
>
>                   /* Load the new GDT */
>      -            State->Gdtr.Size = *((PUSHORT)TableReg);
>      +            // State->Gdtr = TableReg;
>      +            State->Gdtr.Size = *((PUSHORT)&TableReg);
>                   State->Gdtr.Address =
>      *((PULONG)&TableReg[sizeof(USHORT)]);
>
>                   /* In 16-bit mode the highest byte is masked out */
>      @@ -2156,7 +2160,8 @@
>                   }
>
>                   /* Load the new IDT */
>      -            State->Idtr.Size = *((PUSHORT)TableReg);
>      +            // State->Idtr = TableReg;
>      +            State->Idtr.Size = *((PUSHORT)&TableReg);
>                   State->Idtr.Address =
>      *((PULONG)&TableReg[sizeof(USHORT)]);
>
>                   /* In 16-bit mode the highest byte is masked out */
>
>
>