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

[Richard Sandiford <richard dot sandiford at linaro dot org>]

Jeff Law <law@redhat.com> writes:
> On 12/07/2017 07:46 AM, Richard Biener wrote:
>> On Wed, Dec 6, 2017 at 9:11 PM, Jeff Law <law@redhat.com> wrote:
>>> On 11/13/2017 05:04 PM, Richard Sandiford wrote:
>>>> Richard Sandiford <richard.sandiford@linaro.org> writes:
>>>>> Richard Sandiford <richard.sandiford@linaro.org> writes:
>>>>>> 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.
>>>>>
>>>>> Changes in v2:
>>>>>
>>>>> - Drop the controversial known_zero etc. wrapper functions.
>>>>> - Fix the operator<<= bug that Martin found.
>>>>> - Switch from "t" to "type" in SFINAE classes (requested by Martin).
>>>>>
>>>>> Not changed in v2:
>>>>>
>>>>> - Default constructors are still empty.  I agree it makes sense to use
>>>>>   "= default" when we switch to C++11, but it would be dangerous for
>>>>>   that to make "poly_int64 x;" less defined than it is now.
>>>>
>>>> After talking about this a bit more internally, it was obvious that
>>>> the choice of "must" and "may" for the predicate names was a common
>>>> sticking point.  The idea was to match the names of alias predicates,
>>>> but given my track record with names ("too_empty_p" being a recently
>>>> questioned example :-)), I'd be happy to rename them to something else.
>>>> Some alternatives we came up with were:
>>> I didn't find the must vs may naming problematical as I was going
>>> through the changes.  What I did find much more difficult was
>>> determining if the behavior was correct when we used a "may" predicate.
>>> It really relies a good deal on knowing the surrounding code.
>>>
>>> In places where I knew the code reasonably well could tell without much
>>> surrounding context.  In other places I had to look at the code and
>>> deduce proper behavior in the "may" cases -- and often I resorted to
>>> spot checking and relying on your reputation & testing to DTRT.
>>>
>>>
>>>>
>>>> - known_eq / maybe_eq / known_lt / maybe_lt etc.
>>>>
>>>>   Some functions already use "known" and "maybe", so this would arguably
>>>>   be more consistent than using "must" and "may".
>>>>
>>>> - always_eq / sometimes_eq / always_lt / sometimes_lt
>>>>
>>>>   Similar to the previous one in intent.  It's just a question of which
>>>>   wordng is clearer.
>>>>
>>>> - forall_eq / exists_eq / forall_lt / exists_lt etc.
>>>>
>>>>   Matches the usual logic quantifiers.  This seems quite appealing,
>>>>   as long as it's obvious that in:
>>>>
>>>>     forall_eq (v0, v1)
>>>>
>>>>   v0 and v1 themselves are already bound: if vi == ai + bi*X then
>>>>   what we really saying is:
>>>>
>>>>     forall X, a0 + b0*X == a1 + b1*X
>>>>
>>>> Which of those sounds best?  Any other suggestions?
>>> I can live with any of them.  I tend to prefer one of the first two, but
>>> it's not a major concern for me.  So if you or others have a clear
>>> preference, go with it.
>> 
>> Whatever you do use a consistent naming which I guess means
>> using known_eq / maybe_eq?
>> 
>> Otherwise ok.
> So I think that's the final ack on this series.

Thanks to both of you, really appreciate it!

> Richard S. can you confirm?  I fully expect the trunk has moved some
> and the patches will need adjustments -- consider adjustments which
> work in a manner similar to the patches to date pre-approved.

Yeah, that's now all of the poly_int patches.  I still owe you replies
to some of them -- I'll get to that as soon as I can.

I'll make the name changes and propagate through the series and then
commit this first patch.  I was thinking that for the rest it would
make sense to commit them individually, with individual testing of
each patch, so that it's easier to bisect.  I'll try to make sure
I don't repeat the merge mistake in the machine-mode series.

I think it'd also make sense to divide the commits up into groups rather
than do them all at once, since it's easier to do the individual testing
that way.  Does that sound OK?

Thanks,
Richard