Re: [PATCH 1/2] c++: implement __remove_pointer built-in trait

[Patrick Palka <ppalka@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7995 bytes --]

On Sat, 17 Jun 2023, Ken Matsui via Gcc-patches wrote:

> Hi,
> 
> I conducted a benchmark for remove_pointer as well as is_object. Just
> like the is_object benchmark, here is the benchmark code:
> 
> https://github.com/ken-matsui/gcc-benches/blob/main/remove_pointer_benchmark.cc
> 
> On my computer, using the gcc HEAD of this patch for a release build,
> the patch with -DUSE_BUILTIN took 8.7% less time and used 4.3-4.9%
> less memory on average compared to not using it. Although the
> performance improvement was not as significant as with is_object, the
> benchmark demonstrated that the compilation was consistently more
> efficient.

Thanks for the benchmark.  The improvement is lesser than I expected,
but that might be because the benchmark is "biased":

  template <std::size_t N, std::size_t Count = 256>
  struct Instantiator : Instantiator<N, Count - 1> {
      static_assert(!std::is_pointer_v<remove_pointer_t<Instantiator>>);
  };

This only invokes remove_pointer_t on the non-pointer type Instantiator,
and so the benchmark doesn't factor in the performance of the trait when
invoked on pointer types, and traits typically will have different
performance characteristics depending on the kind of type it's given.

To more holistically assess the real-world performance of the trait the
benchmark should also consider pointer types and maybe also cv-qualified
types (given that the original implementation is in terms of
__remove_cv_t and thus performance of the original implementation may be
sensitive to cv-qualification).  So we should probably uniformly
benchmark these classes of types, via doing e.g.:

  static_assert(!std::is_pointer_v<remove_pointer_t<Instantiator>>);
  static_assert(!std::is_pointer_v<remove_pointer_t<Instantiator*>>);
  static_assert(!std::is_pointer_v<remove_pointer_t<const Instantiator>>);
  static_assert(!std::is_pointer_v<remove_pointer_t<Instantiator* const>>);

(We could consider other kinds of types too, e.g. reference types and
integral types, but it seems clear based on the implementations being
benchmarked that performance won't be sensitive to reference-ness
or integral-ness.)

> 
> Sincerely,
> Ken Matsui
> 
> On Thu, Jun 15, 2023 at 5:22 AM Ken Matsui <kmatsui@cs.washington.edu> wrote:
> >
> > This patch implements built-in trait for std::remove_pointer.
> >
> > gcc/cp/ChangeLog:
> >
> >         * cp-trait.def: Define __remove_pointer.
> >         * semantics.cc (finish_trait_type): Handle CPTK_REMOVE_POINTER.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * g++.dg/ext/has-builtin-1.C: Test existence of __remove_pointer.
> >         * g++.dg/ext/remove_pointer.C: New test.
> >
> > Signed-off-by: Ken Matsui <kmatsui@cs.washington.edu>
> > ---
> >  gcc/cp/cp-trait.def                       |  1 +
> >  gcc/cp/semantics.cc                       |  4 ++
> >  gcc/testsuite/g++.dg/ext/has-builtin-1.C  |  3 ++
> >  gcc/testsuite/g++.dg/ext/remove_pointer.C | 51 +++++++++++++++++++++++
> >  4 files changed, 59 insertions(+)
> >  create mode 100644 gcc/testsuite/g++.dg/ext/remove_pointer.C
> >
> > diff --git a/gcc/cp/cp-trait.def b/gcc/cp/cp-trait.def
> > index 8b7fece0cc8..07823e55579 100644
> > --- a/gcc/cp/cp-trait.def
> > +++ b/gcc/cp/cp-trait.def
> > @@ -90,6 +90,7 @@ DEFTRAIT_EXPR (IS_DEDUCIBLE, "__is_deducible ", 2)
> >  DEFTRAIT_TYPE (REMOVE_CV, "__remove_cv", 1)
> >  DEFTRAIT_TYPE (REMOVE_REFERENCE, "__remove_reference", 1)
> >  DEFTRAIT_TYPE (REMOVE_CVREF, "__remove_cvref", 1)
> > +DEFTRAIT_TYPE (REMOVE_POINTER, "__remove_pointer", 1)
> >  DEFTRAIT_TYPE (UNDERLYING_TYPE,  "__underlying_type", 1)
> >  DEFTRAIT_TYPE (TYPE_PACK_ELEMENT, "__type_pack_element", -1)
> >
> > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> > index 8fb47fd179e..885c7a6fb64 100644
> > --- a/gcc/cp/semantics.cc
> > +++ b/gcc/cp/semantics.cc
> > @@ -12373,6 +12373,10 @@ finish_trait_type (cp_trait_kind kind, tree type1, tree type2,
> >        if (TYPE_REF_P (type1))
> >         type1 = TREE_TYPE (type1);
> >        return cv_unqualified (type1);
> > +    case CPTK_REMOVE_POINTER:
> > +      if (TYPE_PTR_P (type1))
> > +    type1 = TREE_TYPE (type1);
> > +      return type1;

Maybe add a newline before the 'case' to visually separate it from the
previous 'case'?  LGTM otherwise, thanks!

> >
> >      case CPTK_TYPE_PACK_ELEMENT:
> >        return finish_type_pack_element (type1, type2, complain);
> > diff --git a/gcc/testsuite/g++.dg/ext/has-builtin-1.C b/gcc/testsuite/g++.dg/ext/has-builtin-1.C
> > index f343e153e56..e21e0a95509 100644
> > --- a/gcc/testsuite/g++.dg/ext/has-builtin-1.C
> > +++ b/gcc/testsuite/g++.dg/ext/has-builtin-1.C
> > @@ -146,3 +146,6 @@
> >  #if !__has_builtin (__remove_cvref)
> >  # error "__has_builtin (__remove_cvref) failed"
> >  #endif
> > +#if !__has_builtin (__remove_pointer)
> > +# error "__has_builtin (__remove_pointer) failed"
> > +#endif
> > diff --git a/gcc/testsuite/g++.dg/ext/remove_pointer.C b/gcc/testsuite/g++.dg/ext/remove_pointer.C
> > new file mode 100644
> > index 00000000000..7b13db93950
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/ext/remove_pointer.C
> > @@ -0,0 +1,51 @@
> > +// { dg-do compile { target c++11 } }
> > +
> > +#define SA(X) static_assert((X),#X)
> > +
> > +SA(__is_same(__remove_pointer(int), int));
> > +SA(__is_same(__remove_pointer(int*), int));
> > +SA(__is_same(__remove_pointer(int**), int*));
> > +
> > +SA(__is_same(__remove_pointer(const int*), const int));
> > +SA(__is_same(__remove_pointer(const int**), const int*));
> > +SA(__is_same(__remove_pointer(int* const), int));
> > +SA(__is_same(__remove_pointer(int** const), int*));
> > +SA(__is_same(__remove_pointer(int* const* const), int* const));
> > +
> > +SA(__is_same(__remove_pointer(volatile int*), volatile int));
> > +SA(__is_same(__remove_pointer(volatile int**), volatile int*));
> > +SA(__is_same(__remove_pointer(int* volatile), int));
> > +SA(__is_same(__remove_pointer(int** volatile), int*));
> > +SA(__is_same(__remove_pointer(int* volatile* volatile), int* volatile));
> > +
> > +SA(__is_same(__remove_pointer(const volatile int*), const volatile int));
> > +SA(__is_same(__remove_pointer(const volatile int**), const volatile int*));
> > +SA(__is_same(__remove_pointer(const int* volatile), const int));
> > +SA(__is_same(__remove_pointer(volatile int* const), volatile int));
> > +SA(__is_same(__remove_pointer(int* const volatile), int));
> > +SA(__is_same(__remove_pointer(const int** volatile), const int*));
> > +SA(__is_same(__remove_pointer(volatile int** const), volatile int*));
> > +SA(__is_same(__remove_pointer(int** const volatile), int*));
> > +SA(__is_same(__remove_pointer(int* const* const volatile), int* const));
> > +SA(__is_same(__remove_pointer(int* volatile* const volatile), int* volatile));
> > +SA(__is_same(__remove_pointer(int* const volatile* const volatile), int* const volatile));
> > +
> > +SA(__is_same(__remove_pointer(int&), int&));
> > +SA(__is_same(__remove_pointer(const int&), const int&));
> > +SA(__is_same(__remove_pointer(volatile int&), volatile int&));
> > +SA(__is_same(__remove_pointer(const volatile int&), const volatile int&));
> > +
> > +SA(__is_same(__remove_pointer(int&&), int&&));
> > +SA(__is_same(__remove_pointer(const int&&), const int&&));
> > +SA(__is_same(__remove_pointer(volatile int&&), volatile int&&));
> > +SA(__is_same(__remove_pointer(const volatile int&&), const volatile int&&));
> > +
> > +SA(__is_same(__remove_pointer(int[3]), int[3]));
> > +SA(__is_same(__remove_pointer(const int[3]), const int[3]));
> > +SA(__is_same(__remove_pointer(volatile int[3]), volatile int[3]));
> > +SA(__is_same(__remove_pointer(const volatile int[3]), const volatile int[3]));
> > +
> > +SA(__is_same(__remove_pointer(int(int)), int(int)));
> > +SA(__is_same(__remove_pointer(int(*const)(int)), int(int)));
> > +SA(__is_same(__remove_pointer(int(*volatile)(int)), int(int)));
> > +SA(__is_same(__remove_pointer(int(*const volatile)(int)), int(int)));
> > --
> > 2.41.0
> >
> 
>