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

[Jeff Law]

On 12/15/2017 02:08 AM, Richard Biener wrote:
> On Fri, Dec 15, 2017 at 4:40 AM, Martin Sebor  wrote:
>> On 12/07/2017 03:48 PM, Jeff Law wrote:
>>>
>>> On 12/07/2017 03:38 PM, Richard Sandiford wrote:
>>>
> So I think that's the final ack on this series.


 Thanks to both of you, really appreciate it!
>>>
>>> Sorry it took so long.
>>>

> 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.
>>>
>>> NP.  I don't think any of the questions were all that significant.
>>> Those which were I think you already responded to.
>>
>>
>> I am disappointed that the no-op ctor issue hasn't been adequately
>> addressed.  No numbers were presented as to the difference it makes
>> to have the ctor do the expected thing (i.e., initialize the object).
>> In my view, the choice seems arbitrarily in favor of a hypothetical
>> performance improvement at -O0 without regard to the impact on
>> correctness.  We have recently seen the adverse effects of similar
>> choices in other areas: the hash table insertion[*] and the related
>> offset_int initialization.
> 
> As were coming from a C code base not initializing stuff is what I expect.
> I'm really surprised to see lot of default initialization done in places
> where it only hurts compile-time (of GCC at least where we need to
> optimize that away).
I suspect a lot of the default initializations were done when Kaveh and
others were working to get us -Wuninitialized clean -- which happened
when uninitialized warnings were still done in RTL (flow.c).

I've long wished we had marked the initializations which were done
solely to make -Wuninitialized happy because it would be a good way to
measure progress on our analysis & optimization passes's ability to
prove the paths weren't executable.

WRT the nop ctor issue, I had a slight leaning towards initializing
them, but I certainly could argue either side.  I think the long term
goal really should be to move to C++11 where it can be done right.

jeff

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

[Richard Biener]

On Fri, Dec 15, 2017 at 4:40 AM, Martin Sebor  wrote:
> On 12/07/2017 03:48 PM, Jeff Law wrote:
>>
>> On 12/07/2017 03:38 PM, Richard Sandiford wrote:
>>
 So I think that's the final ack on this series.
>>>
>>>
>>> Thanks to both of you, really appreciate it!
>>
>> Sorry it took so long.
>>
>>>
 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.
>>
>> NP.  I don't think any of the questions were all that significant.
>> Those which were I think you already responded to.
>
>
> I am disappointed that the no-op ctor issue hasn't been adequately
> addressed.  No numbers were presented as to the difference it makes
> to have the ctor do the expected thing (i.e., initialize the object).
> In my view, the choice seems arbitrarily in favor of a hypothetical
> performance improvement at -O0 without regard to the impact on
> correctness.  We have recently seen the adverse effects of similar
> choices in other areas: the hash table insertion[*] and the related
> offset_int initialization.

As were coming from a C code base not initializing stuff is what I expect.
I'm really surprised to see lot of default initialization done in places
where it only hurts compile-time (of GCC at least where we need to
optimize that away).

Richard.

> Martin
>
> [*] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82977
>
> PS To be clear, the numbers I asked for were those showing
> the difference between a no-op ctor and one that initializes
> the object to some determinate state, whatever that is.  IIUC
> the numbers in the following post show the aggregate slowdown
> for many or most of the changes in the series, not just
> the ctor.  If the numbers were significant, I suggested
> a solution to explicitly request a non-op ctor to make
> the default safe and eliminate the overhead where it mattered.
>
> https://gcc.gnu.org/ml/gcc-patches/2017-11/msg01028.html

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

[Martin Sebor]

On 12/07/2017 03:48 PM, Jeff Law wrote:

On 12/07/2017 03:38 PM, Richard Sandiford wrote:


So I think that's the final ack on this series.


Thanks to both of you, really appreciate it!

Sorry it took so long.




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.

NP.  I don't think any of the questions were all that significant.
Those which were I think you already responded to.


I am disappointed that the no-op ctor issue hasn't been adequately
addressed.  No numbers were presented as to the difference it makes
to have the ctor do the expected thing (i.e., initialize the object).
In my view, the choice seems arbitrarily in favor of a hypothetical
performance improvement at -O0 without regard to the impact on
correctness.  We have recently seen the adverse effects of similar
choices in other areas: the hash table insertion[*] and the related
offset_int initialization.

Martin

[*] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82977

PS To be clear, the numbers I asked for were those showing
the difference between a no-op ctor and one that initializes
the object to some determinate state, whatever that is.  IIUC
the numbers in the following post show the aggregate slowdown
for many or most of the changes in the series, not just
the ctor.  If the numbers were significant, I suggested
a solution to explicitly request a non-op ctor to make
the default safe and eliminate the overhead where it mattered.

https://gcc.gnu.org/ml/gcc-patches/2017-11/msg01028.html

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

[Jeff Law]

On 12/07/2017 03:38 PM, Richard Sandiford wrote:

>> So I think that's the final ack on this series.
> 
> Thanks to both of you, really appreciate it!
Sorry it took so long.

> 
>> 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.
NP.  I don't think any of the questions were all that significant.
Those which were I think you already responded to.

> 
> 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?
Your call on the best way to stage in.

jeff

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

[Richard Sandiford]

Jeff Law  writes:
> On 12/07/2017 07:46 AM, Richard Biener wrote:
>> On Wed, Dec 6, 2017 at 9:11 PM, Jeff Law  wrote:
>>> On 11/13/2017 05:04 PM, Richard Sandiford wrote:
 Richard Sandiford  writes:
> Richard Sandiford  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 == 

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

[Jeff Law]

On 12/07/2017 07:46 AM, Richard Biener wrote:
> On Wed, Dec 6, 2017 at 9:11 PM, Jeff Law  wrote:
>> On 11/13/2017 05:04 PM, Richard Sandiford wrote:
>>> Richard Sandiford  writes:
 Richard Sandiford  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 

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

[Richard Biener]

On Wed, Dec 6, 2017 at 9:11 PM, Jeff Law  wrote:
> On 11/13/2017 05:04 PM, Richard Sandiford wrote:
>> Richard Sandiford  writes:
>>> Richard Sandiford  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 

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

[Jeff Law]

On 11/13/2017 05:04 PM, Richard Sandiford wrote:
> Richard Sandiford  writes:
>> Richard Sandiford  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.


jeff

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

[Jeff Law]

On 11/13/2017 04:36 PM, Richard Sandiford wrote:
> Jeff Law  writes:
>> On 11/09/2017 04:06 AM, Richard Sandiford wrote:
>>
 Let me say at the outset that I struggle to comprehend that a few
 instructions is even a consideration when not optimizing, especially
 in light of the bug the macro caused that would have been prevented
 by using a function instead.  But...
>>>
>>> Many people still build at -O0 though.  One of the things I was asked
>>> for was the time it takes to build stage 2 with an -O0 stage 1
>>> (where stage 1 would usually be built by the host compiler).
>> I suspect folks are concerned about this because it potentially affects
>> their daily development cycle times.  So they're looking to see if the
>> introduction of the poly types has a significant impact.  It's a
>> legitimate question, particularly for the introduction of low level
>> infrastructure that potentially gets hit a lot.
>>
>> Richard, what were the results of that test (if it's elsewhere in the
>> thread I'll eventually find it...
> 
> On an x86_64 box I got:
> 
> real: +7%
> user: +8.6%
> 
> for building stage2 with an -O0 -g stage1.  For aarch64 with the
> NUM_POLY_INT_COEFFS==2 change it was:
> 
> real: +17%
> user: +20%
> 
> That's obviously not ideal, but C++11 would get rid of some of the
> inefficiencies, once we can switch to that.
Ouch.  But I guess to some extent it has to be expected given what
you've got to do under the hood.


> 
> You've probably already seen this, but it's compile-time neutral on
> x86_64 in terms of running a gcc built with --enable-checking=release,
> within a margin of about [-0.1%, 0.1%].
Good.  Presumably that's because it all just falls out and turns into
wi:: stuff on targets that don't need the poly stuff.


> 
> For aarch64 with NUM_POLY_INT_COEFFS==2, a gcc built with
> --enable-checking=release is ~1% slower when using -g and ~2%
> slower with -O2 -g.
That's not terrible given what's going on here.


I'm still pondering the whole construction/initialization and temporary
objects issue.I may try to work through some of the actual patches,
then come back to those issues.


> 
>> I'm just starting to try and make some headway on this kit).
> 
> Thanks :-)  I guess it's going to be a real slog going through them,
> sorry, even despite the attempt to split them up.
No worries.  It's what we sign up for :-)  Your deep testing and long
history with the project really help in that if something goes wrong I
know you're going to be around to fix it.

Jeff

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

[Richard Sandiford]

Martin Sebor  writes:
> On 11/13/2017 04:36 PM, Richard Sandiford wrote:
>> Jeff Law  writes:
>>> On 11/09/2017 04:06 AM, Richard Sandiford wrote:
>>>
> Let me say at the outset that I struggle to comprehend that a few
> instructions is even a consideration when not optimizing, especially
> in light of the bug the macro caused that would have been prevented
> by using a function instead.  But...

 Many people still build at -O0 though.  One of the things I was asked
 for was the time it takes to build stage 2 with an -O0 stage 1
 (where stage 1 would usually be built by the host compiler).
>>> I suspect folks are concerned about this because it potentially affects
>>> their daily development cycle times.  So they're looking to see if the
>>> introduction of the poly types has a significant impact.  It's a
>>> legitimate question, particularly for the introduction of low level
>>> infrastructure that potentially gets hit a lot.
>>>
>>> Richard, what were the results of that test (if it's elsewhere in the
>>> thread I'll eventually find it...
>>
>> On an x86_64 box I got:
>>
>> real: +7%
>> user: +8.6%
>>
>> for building stage2 with an -O0 -g stage1.  For aarch64 with the
>> NUM_POLY_INT_COEFFS==2 change it was:
>>
>> real: +17%
>> user: +20%
>>
>> That's obviously not ideal, but C++11 would get rid of some of the
>> inefficiencies, once we can switch to that.
>
> For the purposes of this discussion, what would the numbers look
> like if the macro were replaced with the inline function as I
> suggested?
>
> What impact on the numbers would having the default ctor actually
> initialize the object have? (As opposed to leaving it uninitialized.)

I was objecting to that for semantic reasons[*], not performance when
built with -O0.  I realise you don't agree, but I don't think either of
us is going to convince the other here.

> I don't want to make a bigger deal out of this macro than it
> already is.  Unlike the wide int constructors, it's
> an implementation detail that, when correct, almost no-one will
> have to worry about.  The main reason for my strenuous objections
> is not the macro itself but the philosophy that performance,
> especially at -O0, should be an overriding consideration.  Code
> should be safe first and foremost.  Otherwise, the few cycles we
> might save by writing unsafe but fast code will be wasted in
> debugging sessions.

But the macro was originally added to improve release builds,
not -O0 builds.  It replaced plain assignments of the form:

  r.coeffs[0] = ...;

Using an inline function instead of a macro is no better than
the original call to operator=(); see the mem_ref_offset figures
I gave earlier.

Thanks,
Richard


[*] Which were:

1) Not all types used with poly_int have a single meaningful initial
   value (wide_int).

2) It prevents useful static warnings about uninitialised variables.
   The fact that we don't warn in every case doesn't defeat this IMO.

3) Using C++11 "= default" is a good compromise, but making poly_ints
   always initialised by default now would make it too dangerous to
   switch to "= default" in future.

4) Conditionally using "= default" when being built with C++11
   compilers and something else when being built with C++03 compilers
   would be too dangerous, since we don't want the semantics of the
   class to depend on host compiler.

5) AFAIK, the only case that would be handled differently by the
   current "() {}" constructor and "= default" is the use of
   "T ()" to construct a zeroed T.  IMO, "T (0)" is better than "T ()"
   anyway because (a) it makes it obvious that you're initialising it to
   the numerical value 0, rather than simply zeroed memory contents, and
   (b) it will give a compile error if T doesn't have a single zero
   representation (as for wide_int).

Those are all independent of whatever the -O0 performance regression
would be from unconditional initialisation.

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

[Martin Sebor]

On 11/13/2017 04:36 PM, Richard Sandiford wrote:

Jeff Law  writes:

On 11/09/2017 04:06 AM, Richard Sandiford wrote:


Let me say at the outset that I struggle to comprehend that a few
instructions is even a consideration when not optimizing, especially
in light of the bug the macro caused that would have been prevented
by using a function instead.  But...


Many people still build at -O0 though.  One of the things I was asked
for was the time it takes to build stage 2 with an -O0 stage 1
(where stage 1 would usually be built by the host compiler).

I suspect folks are concerned about this because it potentially affects
their daily development cycle times.  So they're looking to see if the
introduction of the poly types has a significant impact.  It's a
legitimate question, particularly for the introduction of low level
infrastructure that potentially gets hit a lot.

Richard, what were the results of that test (if it's elsewhere in the
thread I'll eventually find it...


On an x86_64 box I got:

real: +7%
user: +8.6%

for building stage2 with an -O0 -g stage1.  For aarch64 with the
NUM_POLY_INT_COEFFS==2 change it was:

real: +17%
user: +20%

That's obviously not ideal, but C++11 would get rid of some of the
inefficiencies, once we can switch to that.


For the purposes of this discussion, what would the numbers look
like if the macro were replaced with the inline function as I
suggested?

What impact on the numbers would having the default ctor actually
initialize the object have? (As opposed to leaving it uninitialized.)

I don't want to make a bigger deal out of this macro than it
already is.  Unlike the wide int constructors, it's
an implementation detail that, when correct, almost no-one will
have to worry about.  The main reason for my strenuous objections
is not the macro itself but the philosophy that performance,
especially at -O0, should be an overriding consideration.  Code
should be safe first and foremost.  Otherwise, the few cycles we
might save by writing unsafe but fast code will be wasted in
debugging sessions.

Martin

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

[Richard Sandiford]

Richard Sandiford  writes:
> Richard Sandiford  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:

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

Thanks,
Richard

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

[Richard Sandiford]

Jeff Law  writes:
> On 11/09/2017 04:06 AM, Richard Sandiford wrote:
>
>>> Let me say at the outset that I struggle to comprehend that a few
>>> instructions is even a consideration when not optimizing, especially
>>> in light of the bug the macro caused that would have been prevented
>>> by using a function instead.  But...
>> 
>> Many people still build at -O0 though.  One of the things I was asked
>> for was the time it takes to build stage 2 with an -O0 stage 1
>> (where stage 1 would usually be built by the host compiler).
> I suspect folks are concerned about this because it potentially affects
> their daily development cycle times.  So they're looking to see if the
> introduction of the poly types has a significant impact.  It's a
> legitimate question, particularly for the introduction of low level
> infrastructure that potentially gets hit a lot.
>
> Richard, what were the results of that test (if it's elsewhere in the
> thread I'll eventually find it...

On an x86_64 box I got:

real: +7%
user: +8.6%

for building stage2 with an -O0 -g stage1.  For aarch64 with the
NUM_POLY_INT_COEFFS==2 change it was:

real: +17%
user: +20%

That's obviously not ideal, but C++11 would get rid of some of the
inefficiencies, once we can switch to that.

You've probably already seen this, but it's compile-time neutral on
x86_64 in terms of running a gcc built with --enable-checking=release,
within a margin of about [-0.1%, 0.1%].

For aarch64 with NUM_POLY_INT_COEFFS==2, a gcc built with
--enable-checking=release is ~1% slower when using -g and ~2%
slower with -O2 -g.

> I'm just starting to try and make some headway on this kit).

Thanks :-)  I guess it's going to be a real slog going through them,
sorry, even despite the attempt to split them up.

Richard

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

[Jeff Law]

On 11/09/2017 04:06 AM, Richard Sandiford wrote:

>> Let me say at the outset that I struggle to comprehend that a few
>> instructions is even a consideration when not optimizing, especially
>> in light of the bug the macro caused that would have been prevented
>> by using a function instead.  But...
> 
> Many people still build at -O0 though.  One of the things I was asked
> for was the time it takes to build stage 2 with an -O0 stage 1
> (where stage 1 would usually be built by the host compiler).
I suspect folks are concerned about this because it potentially affects
their daily development cycle times.  So they're looking to see if the
introduction of the poly types has a significant impact.  It's a
legitimate question, particularly for the introduction of low level
infrastructure that potentially gets hit a lot.

Richard, what were the results of that test (if it's elsewhere in the
thread I'll eventually find it...  I'm just starting to try and make
some headway on this kit).

Jeff

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

[Martin Sebor]

On 11/09/2017 04:06 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 11:28 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 09:51 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 02:32 AM, Richard Sandiford wrote:

Martin Sebor  writes:

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::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
+inline poly_int_pod&
+poly_int_pod::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.


Good catch!  It should simply have been doing <<= on each coefficient --
I must have got carried away when converting to POLY_SET_COEFF.

I double-checked the other uses and think that's the only one.


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


That would slow down -O0 builds though, by introducing an extra
function call and set of temporaries even when the coefficients
are primitive integers.


Would decorating the function template with attribute always_inline
help?


It would remove the call itself, but we'd still have the extra temporary
objects that were the function argument and return value.


Sorry, I do not want to get into another long discussion about
trade-offs between safety and efficiency but I'm not sure I see
what extra temporaries it would create.  It seems to me that
an inline function template that took arguments of user-defined
types by reference and others by value should be just as efficient
as a macro.

 From GCC's own manual:

   6.43 An Inline Function is As Fast As a Macro
   https://gcc.gnu.org/onlinedocs/gcc/Inline.html


You can see the difference with something like:

  inline
  void __attribute__((always_inline))
  f(int , const int ) { dst = src; }

  int g1(const int ) { int x; f(x, y); return x; }
  int g2(const int ) { int x; x = y; return x; }


Let me say at the outset that I struggle to comprehend that a few
instructions is even a consideration when not optimizing, especially
in light of the bug the macro caused that would have been prevented
by using a function instead.  But...


Many people still build at -O0 though.  One of the things I was asked
for was the time it takes to build stage 2 with an -O0 stage 1
(where stage 1 would usually be built by the host compiler).


Yes, of course.  I do all my development and basic testing at
-O0.  But I don't expect the performance to be comparable to
-O2.  I'd be surprised if anyone did.  What I do expect at -O0
(and what I'm grateful for) is GCC to make it easier to find
bugs in my code by enabling extra checks, even if they come at
the expense of slower execution.

That said, if your enhancement has such dramatic performance
implications at -O0 that the only way to avoid them is by using
macros then I would say it's not appropriate.


...I don't think your example above is representative of using
the POLY_SET_COEFF macro.  The function template I'm suggesting
might look something to this:

   template 
   inline void __attribute__ ((always_inline))
   poly_set_coeff (poly_int_pod *p, unsigned idx, C val)
   {
 ((void) (&(*p).coeffs[0] == (C *) 0),
wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void)
((*p).coeffs[0] = val) : (void) ((*p).coeffs[0].~C (), new
(&(*p).coeffs[0]) C (val)));

 if (N >= 2)
   for (unsigned int i = 1; i < N; i++)
 ((void) (&(*p).coeffs[0] == (C *) 0),
wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void)
((*p).coeffs[i] = val) : (void) ((*p).coeffs[i].~C (), new
(&(*p).coeffs[i]) C (val)));
   }


That ignores the idx parameter and sets all coefficents to val.  Did you
mean somnething like:

   template 
   inline void __attribute__ ((always_inline))
   poly_set_coeff (poly_int_pod *p, unsigned idx, C2 val)
   {
 wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void) 
((*p).coeffs[idx] = val) : (void) ((*p).coeffs[idx].~C1 (), new (&(*p).coeffs[idx]) 
C1 (val));
   }

?  If so...


Yes, I didn't have it 

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

[Richard Sandiford]

Martin Sebor  writes:
> On 11/08/2017 11:28 AM, Richard Sandiford wrote:
>> Martin Sebor  writes:
>>> On 11/08/2017 09:51 AM, Richard Sandiford wrote:
 Martin Sebor  writes:
> On 11/08/2017 02:32 AM, Richard Sandiford wrote:
>> Martin Sebor  writes:
>>> 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::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
>>> +inline poly_int_pod&
>>> +poly_int_pod::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.
>>
>> Good catch!  It should simply have been doing <<= on each coefficient --
>> I must have got carried away when converting to POLY_SET_COEFF.
>>
>> I double-checked the other uses and think that's the only one.
>>
>>> 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.)
>>
>> That would slow down -O0 builds though, by introducing an extra
>> function call and set of temporaries even when the coefficients
>> are primitive integers.
>
> Would decorating the function template with attribute always_inline
> help?

 It would remove the call itself, but we'd still have the extra temporary
 objects that were the function argument and return value.
>>>
>>> Sorry, I do not want to get into another long discussion about
>>> trade-offs between safety and efficiency but I'm not sure I see
>>> what extra temporaries it would create.  It seems to me that
>>> an inline function template that took arguments of user-defined
>>> types by reference and others by value should be just as efficient
>>> as a macro.
>>>
>>>  From GCC's own manual:
>>>
>>>6.43 An Inline Function is As Fast As a Macro
>>>https://gcc.gnu.org/onlinedocs/gcc/Inline.html
>>
>> You can see the difference with something like:
>>
>>   inline
>>   void __attribute__((always_inline))
>>   f(int , const int ) { dst = src; }
>>
>>   int g1(const int ) { int x; f(x, y); return x; }
>>   int g2(const int ) { int x; x = y; return x; }
>
> Let me say at the outset that I struggle to comprehend that a few
> instructions is even a consideration when not optimizing, especially
> in light of the bug the macro caused that would have been prevented
> by using a function instead.  But...

Many people still build at -O0 though.  One of the things I was asked
for was the time it takes to build stage 2 with an -O0 stage 1
(where stage 1 would usually be built by the host compiler).

> ...I don't think your example above is representative of using
> the POLY_SET_COEFF macro.  The function template I'm suggesting
> might look something to this:
>
>template 
>inline void __attribute__ ((always_inline))
>poly_set_coeff (poly_int_pod *p, unsigned idx, C val)
>{
>  ((void) (&(*p).coeffs[0] == (C *) 0), 
> wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void) 
> ((*p).coeffs[0] = val) : (void) ((*p).coeffs[0].~C (), new 
> (&(*p).coeffs[0]) C (val)));
>
>  if (N >= 2)
>for (unsigned int i = 1; i < N; i++)
>  ((void) (&(*p).coeffs[0] == (C *) 0), 
> wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void) 
> ((*p).coeffs[i] = val) : (void) ((*p).coeffs[i].~C (), new 
> (&(*p).coeffs[i]) C (val)));
>}

That ignores the idx parameter and sets all coefficents to val.  Did you
mean somnething like:

   template 
   inline void __attribute__ ((always_inline))
   poly_set_coeff (poly_int_pod *p, unsigned idx, C2 val)
   {
 wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void) 
((*p).coeffs[idx] = val) : (void) ((*p).coeffs[idx].~C1 (), new 
(&(*p).coeffs[idx]) C1 (val));
   }

?  If so...

> To compare apples to apples I suggest to instead compare the shift
> operator (or any other poly_int function that uses the macro) 

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

[Martin Sebor]

On 11/08/2017 11:28 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 09:51 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 02:32 AM, Richard Sandiford wrote:

Martin Sebor  writes:

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::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
+inline poly_int_pod&
+poly_int_pod::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.


Good catch!  It should simply have been doing <<= on each coefficient --
I must have got carried away when converting to POLY_SET_COEFF.

I double-checked the other uses and think that's the only one.


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


That would slow down -O0 builds though, by introducing an extra
function call and set of temporaries even when the coefficients
are primitive integers.


Would decorating the function template with attribute always_inline
help?


It would remove the call itself, but we'd still have the extra temporary
objects that were the function argument and return value.


Sorry, I do not want to get into another long discussion about
trade-offs between safety and efficiency but I'm not sure I see
what extra temporaries it would create.  It seems to me that
an inline function template that took arguments of user-defined
types by reference and others by value should be just as efficient
as a macro.

 From GCC's own manual:

   6.43 An Inline Function is As Fast As a Macro
   https://gcc.gnu.org/onlinedocs/gcc/Inline.html


You can see the difference with something like:

  inline
  void __attribute__((always_inline))
  f(int , const int ) { dst = src; }

  int g1(const int ) { int x; f(x, y); return x; }
  int g2(const int ) { int x; x = y; return x; }


Let me say at the outset that I struggle to comprehend that a few
instructions is even a consideration when not optimizing, especially
in light of the bug the macro caused that would have been prevented
by using a function instead.  But...

...I don't think your example above is representative of using
the POLY_SET_COEFF macro.  The function template I'm suggesting
might look something to this:

  template 
  inline void __attribute__ ((always_inline))
  poly_set_coeff (poly_int_pod *p, unsigned idx, C val)
  {
((void) (&(*p).coeffs[0] == (C *) 0), 
wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void) 
((*p).coeffs[0] = val) : (void) ((*p).coeffs[0].~C (), new 
(&(*p).coeffs[0]) C (val)));


if (N >= 2)
  for (unsigned int i = 1; i < N; i++)
((void) (&(*p).coeffs[0] == (C *) 0), 
wi::int_traits::precision_type == wi::FLEXIBLE_PRECISION ? (void) 
((*p).coeffs[i] = val) : (void) ((*p).coeffs[i].~C (), new 
(&(*p).coeffs[i]) C (val)));

  }

To compare apples to apples I suggest to instead compare the shift
operator (or any other poly_int function that uses the macro) that
doesn't suffer from the bug vs one that makes use of the function
template.  I see a difference of 2 instructions on x86_64 (21 vs
23) for operator<<=.

Are two assembly instructions even worth talking about?


If that's not the case and there is a significant performance
penalty associated with inline functions at -O0 then GCC should
be fixed to avoid it.


I think those docs are really talking about inline functions being as
fast as macros when optimisation is enabled.  I don't think we make
any guarantees about -O0 code quality.


Sure, but you are using unsafe macros in preference to a safer
inline function even with optimization, introducing a bug as
a result, and making an argument that the performance impact
of a few instructions when not using optimization is what should
drive the decision between one and the other in all situations.
With all respect, I fail to see the logic in this like of
reasoning.  By that argument we would never be able to define
any inline functions.

That being said, if the performance implications of using inline
functions with no optimization are so serious 

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

[Richard Sandiford]

Martin Sebor  writes:
> On 11/08/2017 09:51 AM, Richard Sandiford wrote:
>> Martin Sebor  writes:
>>> On 11/08/2017 02:32 AM, Richard Sandiford wrote:
 Martin Sebor  writes:
> 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::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
> +inline poly_int_pod&
> +poly_int_pod::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.

 Good catch!  It should simply have been doing <<= on each coefficient --
 I must have got carried away when converting to POLY_SET_COEFF.

 I double-checked the other uses and think that's the only one.

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

 That would slow down -O0 builds though, by introducing an extra
 function call and set of temporaries even when the coefficients
 are primitive integers.
>>>
>>> Would decorating the function template with attribute always_inline
>>> help?
>>
>> It would remove the call itself, but we'd still have the extra temporary
>> objects that were the function argument and return value.
>
> Sorry, I do not want to get into another long discussion about
> trade-offs between safety and efficiency but I'm not sure I see
> what extra temporaries it would create.  It seems to me that
> an inline function template that took arguments of user-defined
> types by reference and others by value should be just as efficient
> as a macro.
>
>  From GCC's own manual:
>
>6.43 An Inline Function is As Fast As a Macro
>https://gcc.gnu.org/onlinedocs/gcc/Inline.html

You can see the difference with something like:

  inline
  void __attribute__((always_inline))
  f(int , const int ) { dst = src; }

  int g1(const int ) { int x; f(x, y); return x; }
  int g2(const int ) { int x; x = y; return x; }

where *.optimized from GCC 7.1 at -O0 is:

int g1(const int&) (const int & y)
{
  int & dst;
  const int & src;
  int x;
  int D.2285;
  int _3;
  int _6;

   [0.00%]:
  src_5 = y_2(D);
  _6 = *src_5;
  x = _6;
  _3 = x;
  x ={v} {CLOBBER};

 [0.00%]:
  return _3;

}

vs:

int g2(const int&) (const int & y)
{
  int x;
  int D.2288;
  int _4;

   [0.00%]:
  x_3 = *y_2(D);
  _4 = x_3;

 [0.00%]:
  return _4;

}

> If that's not the case and there is a significant performance
> penalty associated with inline functions at -O0 then GCC should
> be fixed to avoid it.

I think those docs are really talking about inline functions being as
fast as macros when optimisation is enabled.  I don't think we make
any guarantees about -O0 code quality.

Thanks,
Richard

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

[Martin Sebor]

On 11/08/2017 09:51 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 02:32 AM, Richard Sandiford wrote:

Martin Sebor  writes:

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::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
+inline poly_int_pod&
+poly_int_pod::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.


Good catch!  It should simply have been doing <<= on each coefficient --
I must have got carried away when converting to POLY_SET_COEFF.

I double-checked the other uses and think that's the only one.


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


That would slow down -O0 builds though, by introducing an extra
function call and set of temporaries even when the coefficients
are primitive integers.


Would decorating the function template with attribute always_inline
help?


It would remove the call itself, but we'd still have the extra temporary
objects that were the function argument and return value.


Sorry, I do not want to get into another long discussion about
trade-offs between safety and efficiency but I'm not sure I see
what extra temporaries it would create.  It seems to me that
an inline function template that took arguments of user-defined
types by reference and others by value should be just as efficient
as a macro.

From GCC's own manual:

  6.43 An Inline Function is As Fast As a Macro
  https://gcc.gnu.org/onlinedocs/gcc/Inline.html

If that's not the case and there is a significant performance
penalty associated with inline functions at -O0 then GCC should
be fixed to avoid it.

Martin

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

[Martin Sebor]

On 11/08/2017 09:51 AM, Richard Sandiford wrote:

Martin Sebor  writes:

On 11/08/2017 02:32 AM, Richard Sandiford wrote:

Martin Sebor  writes:

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::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
+inline poly_int_pod&
+poly_int_pod::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.


Good catch!  It should simply have been doing <<= on each coefficient --
I must have got carried away when converting to POLY_SET_COEFF.

I double-checked the other uses and think that's the only one.


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


That would slow down -O0 builds though, by introducing an extra
function call and set of temporaries even when the coefficients
are primitive integers.


Would decorating the function template with attribute always_inline
help?


It would remove the call itself, but we'd still have the extra temporary
objects that were the function argument and return value.


Sorry, I do not want to get into another long discussion about
trade-offs between safety and efficiency but I'm not sure I see
what extra temporaries it would create.  It seems to me that
an inline function template that took arguments of user-defined
types by reference and others by value should be just as efficient
as a macro.

From GCC's own manual:

  6.43 An Inline Function is As Fast As a Macro
  https://gcc.gnu.org/onlinedocs/gcc/Inline.html

If that's not the case and there is a significant performance
penalty associated with inline functions at -O0 then GCC should
be fixed to avoid it.

Martin

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

[Richard Sandiford]

Martin Sebor  writes:
> On 11/08/2017 02:32 AM, Richard Sandiford wrote:
>> Martin Sebor  writes:
>>> 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::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
>>> +inline poly_int_pod&
>>> +poly_int_pod::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.
>>
>> Good catch!  It should simply have been doing <<= on each coefficient --
>> I must have got carried away when converting to POLY_SET_COEFF.
>>
>> I double-checked the other uses and think that's the only one.
>>
>>> 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.)
>>
>> That would slow down -O0 builds though, by introducing an extra
>> function call and set of temporaries even when the coefficients
>> are primitive integers.
>
> Would decorating the function template with attribute always_inline
> help?

It would remove the call itself, but we'd still have the extra temporary
objects that were the function argument and return value.

Thanks,
Richard

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

[Martin Sebor]

On 11/08/2017 02:32 AM, Richard Sandiford wrote:

Martin Sebor  writes:

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::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
+inline poly_int_pod&
+poly_int_pod::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.


Good catch!  It should simply have been doing <<= on each coefficient --
I must have got carried away when converting to POLY_SET_COEFF.

I double-checked the other uses and think that's the only one.


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


That would slow down -O0 builds though, by introducing an extra
function call and set of temporaries even when the coefficients
are primitive integers.


Would decorating the function template with attribute always_inline
help?

Martin

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

[Richard Sandiford]

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

Tested as before.

Thanks,
Richard


2017-11-08  Richard Sandiford  
Alan Hayward  
David Sherwood  

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.



poly-001-poly-int-h.diff.gz
Description: application/gzip

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

[Richard Sandiford]

Martin Sebor  writes:
> 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::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
> +inline poly_int_pod&
> +poly_int_pod::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.

Good catch!  It should simply have been doing <<= on each coefficient --
I must have got carried away when converting to POLY_SET_COEFF.

I double-checked the other uses and think that's the only one.

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

That would slow down -O0 builds though, by introducing an extra
function call and set of temporaries even when the coefficients
are primitive integers.

> 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
> +struct if_lossless
> +{
> +  typedef T3 t;
> +};

OK, done in v2.

Thanks,
Richard

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

[Martin Sebor]

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  
Alan Hayward  
David Sherwood  

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::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
+inline poly_int_pod&
+poly_int_pod::operator <<= 

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

[Richard Sandiford]

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  
Alan Hayward  
David Sherwood  

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.



poly-int.diff.bz2
Description: BZip2 compressed data