[PATCH] New version of libmpx with new memmove wrapper

[Ilya Enkovich]

2015-11-23 22:44 GMT+03:00 Aleksandra Tsvetkova <astsvetk@gmail.com>:
> gcc/testsuite/ChangeLog
> +2015-10-27  Tsvetkova Alexandra  <aleksandra.tsvetkova@intel.com>
> +
> + * gcc.target/i386/mpx/memmove.c: New test for __mpx_wrapper_memmove.
>
> libmpx/ChangeLog
> +2015-10-28  Tsvetkova Alexandra  <aleksandra.tsvetkova@intel.com>
> +
> + * mpxrt/Makefile.am (libmpx_la_LDFLAGS): Add -version-info option.
> + * libmpxwrap/Makefile.am (libmpx_la_LDFLAGS): Likewise + includes fixed.
> + * libmpx/Makefile.in: Regenerate.
> + * mpxrt/Makefile.in: Regenerate.
> + * libmpxwrap/Makefile.in: Regenerate.
> + * mpxrt/libtool-version: New version.
> + * libmpxwrap/libtool-version: Likewise.
> + * mpxrt/libmpx.map: Add new version and a new symbol.
> + * mpxrt/mpxrt.h: New file.
> + * mpxrt/mpxrt.c (NUM_L1_BITS): Moved to mpxrt.h.
> +                (REG_IP_IDX): Moved to mpxrt.h.
> +                (REX_PREFIX): Moved to mpxrt.h.
> +                (XSAVE_OFFSET_IN_FPMEM): Moved to mpxrt.h.
> +                (MPX_L1_SIZE): Moved to mpxrt.h.

Wrong indentation

> + * libmpxwrap/mpx_wrappers.c: Rewrite __mpx_wrapper_memmove
> + to make it faster.
> + New types: mpx_pointer for extraction of indexes from pointer
> +           mpx_bt_entry represents a cell in bounds table.
> + New functions: alloc_bt for allocatinn bounds table
> +               get_bt to get address of bounds table
> +   copy_if_possible and copy_if_possible_from_end move elements
> +   of bounds table if we can
> +   move_bounds moves bounds just like memmove

Format ChangeLog for this file appropriately.  One entry for function/type.
Look for many examples in existing ChangeLogs.

        (mpx_pointer): New type.
        (alloc_bt): New function.

etc.

>
>
> All fixed except for:
>
>>>+static inline void
>>>+alloc_bt (void *ptr)
>>>+{
>>>+  __asm__ __volatile__ ("bndstx %%bnd0, (%0,%0)"::"r" (ptr):"%bnd0");
>>>+}
>>
>>This should be marked as bnd_legacy.
>
> It will not work.

Why?

>cat test.c
static inline void __attribute__((bnd_legacy))
asm_test (void *ptr)
{
  __asm__ __volatile__ ("bndstx %%bnd0, (%0,%0)"::"r" (ptr):"%bnd0");
}

void
test (void *p1, void *p2)
{
  asm_test (p1);
  asm_test (p2);
}
>gcc test.c -S -mmpx -O2
>cat test.s
        .file   "test.c"
        .text
        .p2align 4,,15
        .globl  test
        .type   test, @function
test:
.LFB1:
        .cfi_startproc
#APP
# 4 "test.c" 1
        bndstx %bnd0, (%rdi,%rdi)
# 0 "" 2
# 4 "test.c" 1
        bndstx %bnd0, (%rsi,%rsi)
# 0 "" 2
#NO_APP
        ret
        .cfi_endproc

Seems to work for me...

>
>> +void *
>> +__mpx_wrapper_memmove (void *dst, const void *src, size_t n)
>> +{
>> +  if (n == 0)
>> +    return dst;
>> +
>> +  __bnd_chk_ptr_bounds (dst, n);
>> +  __bnd_chk_ptr_bounds (src, n);
>> +
>> +  memmove (dst, src, n);
>> +  move_bounds (dst, src, n);
>> +  return dst;
>>  }
>>
>> You completely remove old algorithm which should be faster on small
>> sizes. __mpx_wrapper_memmove should become a dispatcher between old
>> and new implementations depending on target (32-bit or 64-bit) and N.
>> Since old version performs both data and bounds copy, BD check should
>> be moved into __mpx_wrapper_memmove to never call
>> it when MPX is disabled.
>
> Even though the old algorithm is faster on small sizes, it should not be used
> with the new one because the new one supports unaligned pointers and the
> old one does not. Different behavior may cause more problems.

OK, lets stick to the single algorithm and think how to improve it for
small sizes.

+
+const size_t MPX_L1_SIZE = (1UL << NUM_L1_BITS) * sizeof (void *);

Use #define

+  if ((n < sizeof (void *)) || !((char*)src-(char*)dst))

Move this check into __mpx_wrapper_memmove. You also can use || src == dst.

You still didn't describe performed testing.


Ilya

>
> Thanks,
> Alexandra.