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, [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=6...
>
> ==============================================================================
> --- 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, <[e-mail] 
> 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=64773&r1=64772&r2=64773&view=diff
> ==============================================================================
> --- 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=64773&r1=64772&r2=64773&view=diff
> ==============================================================================
> --- 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 */