On Wed, Oct 1, 2014 at 4:09 PM, Jan Hubicka <hubi...@ucw.cz> wrote: >> The block frequencies are very small in this case leading to rounding >> errors when computing the edge frequency. The rounding error was then >> propagated into the recomputed probabilities, leading to insanities in >> the outgoing edge probabilities on the jump threading path. Eventually >> during rtl expansion these insanities somehow caused an ICE. > > Concerning the never ending rounoff issues, I thing we ought to progress > on turning counts, freqs and probabilities into a type that eliminates > most of this fun. > > I see basically two options - first would be to take wide_int and build > a fixed point type template around it (so one can chose precisions), > other would be to have software emulated floating point type. > > Actually I think the second will be less troublesome - FP seems to fit > the bill very well. We already have sreal.h that does the job for > propagation of frequencies. What about turning it ito C++ type with > operations on it and reroganizing code to use it? > (we may consider making the base 64bit, as opposed to HOST_WIDE_INT > right now) > > Or are there other options I missed? It would be very cool if someone > could volunteer and implement the basic datatype and possibly start with > convertion. > > I think we could do type by type, first redefining counts from gcov_t > and relying on conversion to gcov_t to work to not having to update all > uses at once. THen we can just update the code pass by pass. > > One nice thing we could add into the type is to make difference in betwen > known zeros (i.e. read from profile) and zeros that were results of updates > that may or may not be precise. THis should hopefully solve the bad > iteraction of Martin's code ordering and Theresa's BB-reorder work.
Yes, this would avoid a number of headaches with roundoff errors I've seen and worked around in various contexts. Using C++ to implement the sreal type and provide the operators seems like a good option. I had hoped to work on this after we talked about it in the past but haven't had the bandwidth unfortunately. Teresa > > Honza >> >> (Before this patch the probabilities weren't even being updated, >> leading to insanities in other cases where they needed to be updated.) >> >> Specifically, in this case we had a block with frequency = 1. It had >> two outgoing edges each with probability 50%. Because the >> EDGE_FREQUENCY computation uses a rounding divide, the outgoing edge >> frequencies were both computed as 1. Later use of these block and edge >> frequencies to recompute the probability lead to both outgoing edges >> getting 100%. >> >> To address this, in the routine freqs_to_counts_path can simply scale >> up the frequencies when recording them in the count field. I added a >> scale by REG_BR_PROB_BASE. The largest count possible would therefore >> be REG_BR_PROB_BASE * BB_FREQ_MAX which is 10000 * 10000 = 100mil >> which safely fits in gcov_type. >> >> Here is the patch I am testing. Confirmed it fixes the reported issue. >> Currently bootstrapping and testing on x86_64-unknown-linux-gnu. Ok >> for trunk if it passes? >> >> (The whitespace is getting messed up when I copy the patch in here - >> the indentations do line up in the patch.) >> >> Thanks, >> Teresa >> >> 2014-10-01 Teresa Johnson <tejohn...@google.com> >> >> * tree-ssa-threadupdate.c (freqs_to_counts_path): Scale frequencies >> up when synthesizing counts to avoid rounding errors. >> >> Index: tree-ssa-threadupdate.c >> =================================================================== >> --- tree-ssa-threadupdate.c (revision 215739) >> +++ tree-ssa-threadupdate.c (working copy) >> @@ -979,7 +979,11 @@ freqs_to_counts_path (struct redirection_data *rd) >> FOR_EACH_EDGE (ein, ei, e->dest->preds) >> { >> gcc_assert (!ein->count); >> - ein->count = EDGE_FREQUENCY (ein); >> + /* Scale up the frequency by REG_BR_PROB_BASE, to avoid rounding >> + errors applying the probability when the frequencies are very >> + small. */ >> + ein->count = apply_probability (ein->src->frequency * >> REG_BR_PROB_BASE, >> + ein->probability); >> } >> >> for (unsigned int i = 1; i < path->length (); i++) >> @@ -987,11 +991,13 @@ freqs_to_counts_path (struct redirection_data *rd) >> edge epath = (*path)[i]->e; >> gcc_assert (!epath->count); >> edge esucc; >> + /* Scale up the frequency by REG_BR_PROB_BASE, to avoid rounding >> + errors applying the edge probability when the frequencies are very >> + small. */ >> + epath->src->count = epath->src->frequency * REG_BR_PROB_BASE; >> FOR_EACH_EDGE (esucc, ei, epath->src->succs) >> - { >> - esucc->count = EDGE_FREQUENCY (esucc); >> - } >> - epath->src->count = epath->src->frequency; >> + esucc->count = apply_probability (esucc->src->count, >> + esucc->probability); >> } >> } >> >> >> On Wed, Oct 1, 2014 at 8:29 AM, Teresa Johnson <tejohn...@google.com> wrote: >> > I got the preprocessed source. With the aarch64 cross-compiler I built >> > I am able to reproduce the ICE. Looking at it now. >> > >> > Thanks, >> > Teresa >> > >> > On Wed, Oct 1, 2014 at 8:25 AM, Christophe Lyon >> > <christophe.l...@linaro.org> wrote: >> >> On 1 October 2014 17:22, Sebastian Pop <seb...@gmail.com> wrote: >> >>> Christophe Lyon wrote: >> >>>> Since this commit, I can see all my builds for arm*linux* and >> >>>> aarch64*linux* fail while building glibc: >> >>>> >> >>>> /tmp/3496222_18.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/bin/aarch64-none-linux-gnu-gcc >> >>>> iso-2022-cn.c -c -std=gnu99 -fgnu89-inline -O2 -Wall -Win >> >>>> line -Wundef -Wwrite-strings -fmerge-all-constants -frounding-math -g >> >>>> -Wstrict-prototypes -fPIC -I../include >> >>>> -I/tmp/3496222_18.tmpdir/aci-gcc-f >> >>>> sf/builds/gcc-fsf-gccsrc/obj-aarch64-none-linux-gnu/glibc-1/iconvdata >> >>>> -I/tmp/3496222_18.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64-none-linux >> >>>> -gnu/glibc-1 -I../sysdeps/unix/sysv/linux/aarch64/nptl >> >>>> -I../sysdeps/unix/sysv/linux/aarch64 >> >>>> -I../sysdeps/unix/sysv/linux/generic -I../sysdeps/unix/s >> >>>> ysv/linux/wordsize-64 -I../nptl/sysdeps/unix/sysv/linux >> >>>> -I../nptl/sysdeps/pthread -I../sysdeps/pthread >> >>>> -I../sysdeps/unix/sysv/linux -I../sysdeps/gn >> >>>> u -I../sysdeps/unix/inet -I../nptl/sysdeps/unix/sysv >> >>>> -I../sysdeps/unix/sysv -I../nptl/sysdeps/unix -I../sysdeps/unix >> >>>> -I../sysdeps/posix -I../sysd >> >>>> eps/aarch64/fpu -I../sysdeps/aarch64/nptl -I../sysdeps/aarch64 >> >>>> -I../sysdeps/wordsize-64 -I../sysdeps/ieee754/ldbl-128 >> >>>> -I../sysdeps/ieee754/dbl-64/w >> >>>> ordsize-64 -I../sysdeps/ieee754/dbl-64 -I../sysdeps/ieee754/flt-32 >> >>>> -I../sysdeps/aarch64/soft-fp -I../sysdeps/ieee754 >> >>>> -I../sysdeps/generic -I../npt >> >>>> l -I.. -I../libio -I. -nostdinc -isystem >> >>>> /tmp/3496222_18.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/lib/gcc/aarch64-none-linux-gnu/5.0.0/include >> >>>> -i >> >>>> system >> >>>> /tmp/3496222_18.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/tools/lib/gcc/aarch64-none-linux-gnu/5.0.0/include-fixed >> >>>> -isystem /tmp/3496222_18.tmpdir >> >>>> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/sysroot-aarch64-none-linux-gnu/usr/include >> >>>> -D_LIBC_REENTRANT -include ../include/libc-symbols.h -DPIC -DSHARED >> >>>> -DNOT_IN_libc -o >> >>>> /tmp/3496222_18.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64-none-linux-gnu/glibc-1/iconvdata/iso-2022-cn.os >> >>>> -MD -MP -MF /tmp/3 >> >>>> 496222_18.tmpdir/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-aarch64-none-linux-gnu/glibc-1/iconvdata/iso-2022-cn.os.dt >> >>>> -MT /tmp/3496222_18.tmpdir/aci-gcc-fsf >> >>>> /builds/gcc-fsf-gccsrc/obj-aarch64-none-linux-gnu/glibc-1/iconvdata/iso-2022-cn.os >> >>>> >> >>>> In file included from iso-2022-cn.c:407:0: >> >>>> ../iconv/skeleton.c: In function 'gconv': >> >>>> ../iconv/skeleton.c:800:1: internal compiler error: in >> >>>> check_probability, at basic-block.h:959 >> >>>> 0xe4e2fb find_many_sub_basic_blocks(simple_bitmap_def*) >> >>>> >> >>>> /tmp/3496222_18.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/basic-block.h:959 >> >>>> 0x6623f0 execute >> >>>> >> >>>> /tmp/3496222_18.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgexpand.c:5916 >> >>>> Please submit a full bug report, >> >>>> with preprocessed source if appropriate. >> >>>> Please include the complete backtrace with any bug report. >> >>>> See <http://gcc.gnu.org/bugs.html> for instructions. >> >>>> >> >>>> Can you have a look? >> >>> >> >>> It would help if you could attach a preprocessed file. >> >>> >> >> I did it in a followup mail, but the list server rejected it because >> >> it was too large. >> >> I suppose Teresa did receive it though. >> >> >> >> Not sure whether I can attach it in .xz format? Is this allowed? >> >> >> >> Thanks >> >> Christophe. >> >> >> >>> Thanks, >> >>> Sebastian >> > >> > >> > >> > -- >> > Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413