This is the mail archive of the
`gcc-patches@gcc.gnu.org`
mailing list for the GCC project.

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |

Other format: | [Raw text] |

*From*: Richard Biener <richard dot guenther at gmail dot com>*To*: Aldy Hernandez <aldyh at redhat dot com>, Richard Sandiford <rdsandiford at googlemail dot com>*Cc*: GCC Patches <gcc-patches at gcc dot gnu dot org>*Date*: Fri, 6 Jul 2018 16:47:40 +0200*Subject*: Re: calculate overflow type in wide int arithmetic*References*: <8aaeea6d-6a55-e861-dd26-5ee3a364ad4e@redhat.com> <CAFiYyc2DBbACa0i2Q_u1ENiw1X5sUS7zmwYreBTwE54h_iYEaw@mail.gmail.com> <675b172b-0b12-482e-84e6-611d74072859@redhat.com>

On Fri, Jul 6, 2018 at 9:50 AM Aldy Hernandez <aldyh@redhat.com> wrote: > > > > On 07/05/2018 05:50 AM, Richard Biener wrote: > > On Thu, Jul 5, 2018 at 9:35 AM Aldy Hernandez <aldyh@redhat.com> wrote: > >> > >> The reason for this patch are the changes showcased in tree-vrp.c. > >> Basically I'd like to discourage rolling our own overflow and underflow > >> calculation when doing wide int arithmetic. We should have a > >> centralized place for this, that is-- in the wide int code itself ;-). > >> > >> The only cases I care about are plus/minus, which I have implemented, > >> but we also get division for free, since AFAICT, division can only > >> positive overflow: > >> > >> -MIN / -1 => +OVERFLOW > >> > >> Multiplication OTOH, can underflow, but I've not implemented it because > >> we have no uses for it. I have added a note in the code explaining this. > >> > >> Originally I tried to only change plus/minus, but that made code that > >> dealt with plus/minus in addition to div or mult a lot uglier. You'd > >> have to special case "int overflow_for_add_stuff" and "bool > >> overflow_for_everything_else". Changing everything to int, makes things > >> consistent. > >> > >> Note: I have left poly-int as is, with its concept of yes/no for > >> overflow. I can adapt this as well if desired. > >> > >> Tested on x86-64 Linux. > >> > >> OK for trunk? > > > > looks all straight-forward but the following: > > > > else if (op1) > > { > > if (minus_p) > > - { > > - wi = -wi::to_wide (op1); > > - > > - /* Check for overflow. */ > > - if (sgn == SIGNED > > - && wi::neg_p (wi::to_wide (op1)) > > - && wi::neg_p (wi)) > > - ovf = 1; > > - else if (sgn == UNSIGNED && wi::to_wide (op1) != 0) > > - ovf = -1; > > - } > > + wi = wi::neg (wi::to_wide (op1)); > > else > > wi = wi::to_wide (op1); > > > > you fail to handle - -INT_MIN. > > Woah, very good catch. I previously had this implemented as wi::sub(0, > op1, &ovf) which was calculating overflow correctly but when I > implemented the overflow type in wi::neg I missed this. Thanks. > > > > > Given the fact that for multiplication (or others, didn't look too close) > > you didn't implement the direction indicator I wonder if it would be more > > appropriate to do > > > > enum ovfl { OVFL_NONE = 0, OVFL_UNDERFLOW = -1, OVFL_OVERFLOW = 1, > > OVFL_UNKNOWN = 2 }; > > > > and tell us the "truth" here? > > Excellent idea...though it came with lots of typing :). Fixed. > > BTW, if I understand correctly, I've implemented the overflow types > correctly for everything but multiplication (which we have no users for > and I return OVF_UNKNOWN). I have indicated this in comments. Also, > for division I did nothing special, as we can only +OVERFLOW. > > > > > Hopefully if (overflow) will still work with that. > > It does. > > > > > Otherwise can you please add a toplevel comment to wide-int.h as to what the > > overflow result semantically is for a) SIGNED and b) UNSIGNED operations? > > Done. Let me know if the current comment is what you had in mind. > > OK for trunk? I'd move accumulate_overflow to wi::, it looks generally useful. That function misses to handle the !suboverflow && overflow case optimally. I see that poly-int choses to accumulate overflow (callers need to initialize it) while wide_int chooses not to accumulate... to bad this is inconsistent. Richard? OK with the fix/move of accumulate_overflow. Thanks, Richard.

**Follow-Ups**:**Re: calculate overflow type in wide int arithmetic***From:*Richard Sandiford

**References**:**calculate overflow type in wide int arithmetic***From:*Aldy Hernandez

**Re: calculate overflow type in wide int arithmetic***From:*Richard Biener

**Re: calculate overflow type in wide int arithmetic***From:*Aldy Hernandez

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |