This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [001/nnn] poly_int: add poly-int.h


On 10/23/2017 10:57 AM, Richard Sandiford wrote:
This patch adds a new "poly_int" class to represent polynomial integers
of the form:

  C0 + C1*X1 + C2*X2 ... + Cn*Xn

It also adds poly_int-based typedefs for offsets and sizes of various
precisions.  In these typedefs, the Ci coefficients are compile-time
constants and the Xi indeterminates are run-time invariants.  The number
of coefficients is controlled by the target and is initially 1 for all
ports.

Most routines can handle general coefficient counts, but for now a few
are specific to one or two coefficients.  Support for other coefficient
counts can be added when needed.

The patch also adds a new macro, IN_TARGET_CODE, that can be
set to indicate that a TU contains target-specific rather than
target-independent code.  When this macro is set and the number of
coefficients is 1, the poly-int.h classes define a conversion operator
to a constant.  This allows most existing target code to work without
modification.  The main exceptions are:

- values passed through ..., which need an explicit conversion to a
  constant

- ?: expression in which one arm ends up being a polynomial and the
  other remains a constant.  In these cases it would be valid to convert
  the constant to a polynomial and the polynomial to a constant, so a
  cast is needed to break the ambiguity.

The patch also adds a new target hook to return the estimated
value of a polynomial for costing purposes.

The patch also adds operator<< on wide_ints (it was already defined
for offset_int and widest_int).  I think this was originally excluded
because >> is ambiguous for wide_int, but << is useful for converting
bytes to bits, etc., so is worth defining on its own.  The patch also
adds operator% and operator/ for offset_int and widest_int, since those
types are always signed.  These changes allow the poly_int interface to
be more predictable.

I'd originally tried adding the tests as selftests, but that ended up
bloating cc1 by at least a third.  It also took a while to build them
at -O2.  The patch therefore uses plugin tests instead, where we can
force the tests to be built at -O0.  They still run in negligible time
when built that way.


2017-10-23  Richard Sandiford  <richard.sandiford@linaro.org>
	    Alan Hayward  <alan.hayward@arm.com>
	    David Sherwood  <david.sherwood@arm.com>

gcc/
	* poly-int.h: New file.
	* poly-int-types.h: Likewise.
	* coretypes.h: Include them.
	(POLY_INT_CONVERSION): Define.
	* target.def (estimated_poly_value): New hook.
	* doc/tm.texi.in (TARGET_ESTIMATED_POLY_VALUE): New hook.
	* doc/tm.texi: Regenerate.
	* doc/poly-int.texi: New file.
	* doc/gccint.texi: Include it.
	* doc/rtl.texi: Describe restrictions on subreg modes.
	* Makefile.in (TEXI_GCCINT_FILES): Add poly-int.texi.
	* genmodes.c (NUM_POLY_INT_COEFFS): Provide a default definition.
	(emit_insn_modes_h): Emit a definition of NUM_POLY_INT_COEFFS.
	* targhooks.h (default_estimated_poly_value): Declare.
	* targhooks.c (default_estimated_poly_value): New function.
	* target.h (estimated_poly_value): Likewise.
	* wide-int.h (WI_UNARY_RESULT): Use wi::binary_traits.
	(wi::unary_traits): Delete.
	(wi::binary_traits::signed_shift_result_type): Define for
	offset_int << HOST_WIDE_INT, etc.
	(generic_wide_int::operator <<=): Define for all types and use
	wi::lshift instead of <<.
	(wi::hwi_with_prec): Add a default constructor.
	(wi::ints_for): New class.
	(operator <<): Define for all wide-int types.
	(operator /): New function.
	(operator %): Likewise.
	* selftest.h (ASSERT_MUST_EQ, ASSERT_MUST_EQ_AT, ASSERT_MAY_NE)
	(ASSERT_MAY_NE_AT): New macros.

gcc/testsuite/
	* gcc.dg/plugin/poly-int-tests.h,
	gcc.dg/plugin/poly-int-test-1.c,
	gcc.dg/plugin/poly-int-01_plugin.c,
	gcc.dg/plugin/poly-int-02_plugin.c,
	gcc.dg/plugin/poly-int-03_plugin.c,
	gcc.dg/plugin/poly-int-04_plugin.c,
	gcc.dg/plugin/poly-int-05_plugin.c,
	gcc.dg/plugin/poly-int-06_plugin.c,
	gcc.dg/plugin/poly-int-07_plugin.c: New tests.
	* gcc.dg/plugin/plugin.exp: Run them.

I haven't done nearly a thorough review but the dtor followed by
the placement new in the POLY_SET_COEFF() macro caught my eye so
I thought I'd ask sooner rather than later.  Given the macro
definition:

+   The dummy comparison against a null C * is just a way of checking
+   that C gives the right type.  */
+#define POLY_SET_COEFF(C, RES, I, VALUE) \
+  ((void) (&(RES).coeffs[0] == (C *) 0), \
+   wi::int_traits<C>::precision_type == wi::FLEXIBLE_PRECISION \
+   ? (void) ((RES).coeffs[I] = VALUE) \
+   : (void) ((RES).coeffs[I].~C (), new (&(RES).coeffs[I]) C (VALUE)))

is the following use well-defined?

+template<unsigned int N, typename C>
+inline poly_int_pod<N, C>&
+poly_int_pod<N, C>::operator <<= (unsigned int a)
+{
+  POLY_SET_COEFF (C, *this, 0, this->coeffs[0] << a);

It looks to me as though the VALUE argument in the ctor invoked
by the placement new expression is evaluated after the dtor has
destroyed the very array element the VALUE argument expands to.

Or am I misreading the code?

Whether or not is, in fact, a problem, it seems to me that using
a function template rather than a macro would be a clearer and
safer way to do the same thing.  (Safer in that the macro also
evaluates its arguments multiple times, which is often a source
of subtle bugs.)

Other than that, I would suggest changing 't' to something a bit
less terse, like perhaps 'type' in traits like the following:

+struct if_lossless;
+template<typename T1, typename T2, typename T3>
+struct if_lossless<T1, T2, T3, true>
+{
+  typedef T3 t;
+};

Lastly (for now), I note that the default ply_int ctor, like
that of the other xxx_int types, is a no-op.  That makes using
all these types error prone, e.g., as arrays in ctor-initializer
lists:

  struct Pair {
    poly_int<...> poly[2];

    Pair (): poly () { }   // poly[] (unexpectedly) uninitialized
  }

Martin

PS My initial interest in this class was to to see if the it is
less prone to error than wide_int and offset_int.  Specifically,
if it's easier to convert the various flavors of xxx_int among
one another and between basic integers and the xxx_ints.  But
after reading the documentation I have the impression it might
help with some of the range work I've been doing recently, so
I'll try to do a more thorough review in the (hopefully) near
future.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]