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

[Jeff Law <law at redhat dot com>]

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.  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.

jeff