On Wed, Dec 6, 2017 at 9:11 PM, Jeff Law <l...@redhat.com> wrote:
> On 11/13/2017 05:04 PM, Richard Sandiford wrote:
>> Richard Sandiford <richard.sandif...@linaro.org> writes:
>>> Richard Sandiford <richard.sandif...@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.
Richard.
>
> jeff
