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

[Martin Sebor <msebor@gmail.com>]

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.