On Tue, Aug 15, 2017 at 4:21 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 3:52 PM, Martin Jambor <mjam...@suse.cz> wrote:
>> Hi Joseph,
>>
>> On Thu, May 26, 2016 at 09:02:02PM +0000, Joseph Myers wrote:
>>> On Thu, 26 May 2016, Jan Hubicka wrote:
>>>
>>> > > > +ffp-int-builtin-inexact
>>> > > > +Common Report Var(flag_fp_int_builtin_inexact) Optimization
>>> > > > +Allow built-in functions ceil, floor, round, trunc to raise
>>> > > > \"inexact\" exceptions.
>>> >
>>> > When adding new codegen option which affects the correctness, it is also
>>> > necessary to update can_inline_edge_p and inline_call.
>>>
>>> This patch version adds handling for the new option in those places.
>>> Other changes: the default for the option is corrected so that
>>> -ffp-int-builtin-inexact really is in effect by default as intended;
>>> md.texi documentation for the patterns in question is updated to
>>> describe how they are affected by this option.
>>>
>>>
>>> Add option for whether ceil etc. can raise "inexact", adjust x86 conditions.
>>>
>>> In ISO C99/C11, the ceil, floor, round and trunc functions may or may
>>> not raise the "inexact" exception for noninteger arguments. Under TS
>>> 18661-1:2014, the C bindings for IEEE 754-2008, these functions are
>>> prohibited from raising "inexact", in line with the general rule that
>>> "inexact" is only when the mathematical infinite precision result of a
>>> function differs from the result after rounding to the target type.
>>>
>>> GCC has no option to select TS 18661 requirements for not raising
>>> "inexact" when expanding built-in versions of these functions inline.
>>> Furthermore, even given such requirements, the conditions on the x86
>>> insn patterns for these functions are unnecessarily restrictive. I'd
>>> like to make the out-of-line glibc versions follow the TS 18661
>>> requirements; in the cases where this slows them down (the cases using
>>> x87 floating point), that makes it more important for inline versions
>>> to be used when the user does not care about "inexact".
>>
>> Unfortunately, I have found out that this commit regresses run-time of
>> 538.imagick_r by about 5% on an AMD Ryzen machine and by 9% on a
>> slightly older Intel machine when compiled with just -O2 (so with
>> generic tuning).
>>
>> The problem is that ImageMagick spends a lot time calculating ceil and
>> floor and even with with generic tuning their library implementations
>> can use the ifunc mechanism to execute an efficient SSE 4.1
>> implementation on the processors that have it, whereas the inline
>> expansion cannot do so and is much bigger and much much slower. To
>> give you an idea, this is the profile before and after the change:
>>
>> | Symbol | 237073 | % of runtime | 237074 | %
>> of runtime | sample delta | % sample delta |
>>
>> |----------------------------------+---------+--------------+---------+--------------+--------------+----------------|
>> | MorphologyApply | 1058932 | 52.88% | 1043194 |
>> 46.65% | -15738 | 98.51 |
>> | MeanShiftImage | 508088 | 25.50% | 833378 |
>> 37.43% | 325290 | 164.02 |
>> | GetVirtualPixelsFromNexus | 173354 | 8.70% | 168298 |
>> 7.56% | -5056 | 97.08 |
>> | SetPixelCacheNexusPixels.isra.10 | 114101 | 5.72% | 112790 |
>> 5.07% | -1311 | 98.85 |
>> | __ceil_sse41 | 21404 | 1.07% | 0 |
>> 0 | -21404 | 0.00 |
>> | __floor_sse41 | 19179 | 0.96% | 0 |
>> 0 | -19179 | 0.00 |
>>
>> And all of the sample count increases in MeanShiftImage can be tracked
>> down to the line in the cource calculating
>>
>> if ((x-floor(x)) < (ceil(x)-x))
>>
>> I am not sure what to do about this, to me it seems that the
>> -ffp-int-builtin-inexact simply has a wrong default value, at least
>> for x86_64, as it was added in order not to slow code down but does
>> exactly that (all of the slowdown of course disappears when
>> -fno-fp-int-builtin-inexact is used).
>>
>> Or is the situation somehow more complex?
>
> I suppose these days the big inline sequences for the rounding functions
> are no longer profitable for generic tuning (assuming 'generic' nowadays
> includes SSE41 support). Esp. floor/ceil includes jumpy compensation
> code.
Note other options are to inline if (__builtin_cpu_supports
("sse4.1")) ... else ...
or to emit a call to a (local? comdat?) __gcc_floor ifunc dispatcher and
emit the ifunc math library ourselves (like we'd do with attribute(target(""))).
Not sure if we really can assume glibc is intelligent enough -- does it have
non-SSE4.1 implementations for ceil/floor? Back in time I implemented
these SSE2 expansions it used the generic C code which was awfully slow...
Richard.
> Note that (x - floor(x)) < (ceil(x) - x) looks like some clever simplification
> might speed it up. Not that I can come up with sth off my head...
>
> Richard.
>
>> Martin
>>
>>
>>>
>>> This patch fixes these issues. A new option
>>> -fno-fp-int-builtin-inexact is added to request TS 18661 rules for
>>> these functions; the default -ffp-int-builtin-inexact reflects that
>>> such exceptions are allowed by C99 and C11. (The intention is that if
>>> C2x incorporates TS 18661-1, then the default would change in C2x
>>> mode.)
>>>
>>> The x86 built-ins for rint (x87, SSE2 and SSE4.1) are made
>>> unconditionally available (no longer depending on
>>> -funsafe-math-optimizations or -fno-trapping-math); "inexact" is
>>> correct for noninteger arguments to rint. For floor, ceil and trunc,
>>> the x87 and SSE2 built-ins are OK if -ffp-int-builtin-inexact or
>>> -fno-trapping-math (they may raise "inexact" for noninteger
>>> arguments); the SSE4.1 built-ins are made to use ROUND_NO_EXC so that
>>> they do not raise "inexact" and so are OK unconditionally.
>>>
>>> Now, while there was no semantic reason for depending on
>>> -funsafe-math-optimizations, the insn patterns had such a dependence
>>> because of use of gen_truncxf<mode>2_i387_noop to truncate back to
>>> SFmode or DFmode after using frndint in XFmode. In this case a no-op
>>> truncation is safe because rounding to integer always produces an
>>> exactly representable value (the same reason why IEEE semantics say it
>>> shouldn't produce "inexact") - but of course that insn pattern isn't
>>> safe because it would also match cases where the truncation is not in
>>> fact a no-op. To allow frndint to be used for SFmode and DFmode
>>> without that unsafe pattern, the relevant frndint patterns are
>>> extended to SFmode and DFmode or new SFmode and DFmode patterns added,
>>> so that the frndint operation can be represented in RTL as an
>>> operation acting directly on SFmode or DFmode without the extension
>>> and the problematic truncation.
>>>
>>> A generic test of the new option is added, as well as x86-specific
>>> tests, both execution tests including the generic test with different
>>> x86 options and scan-assembler tests verifying that functions that
>>> should be inlined with different options are indeed inlined.
>>>
>>> I think other architectures are OK for TS 18661-1 semantics already.
>>> Considering those defining "ceil" patterns: aarch64, arm, rs6000, s390
>>> use instructions that do not raise "inexact"; nvptx does not support
>>> floating-point exceptions. (This does mean the -f option in fact only
>>> affects one architecture, but I think it should still be a -f option;
>>> it's logically architecture-independent and is expected to be affected
>>> by future -std options, so is similar to e.g. -fexcess-precision=,
>>> which also does nothing on most architectures but is implied by -std
>>> options.)
>>>
>>> Bootstrapped with no regressions on x86_64-pc-linux-gnu. OK to
>>> commit?
>>>
>>> gcc:
>>> 2016-05-26 Joseph Myers <jos...@codesourcery.com>
>>>
>>> PR target/71276
>>> PR target/71277
>>> * common.opt (ffp-int-builtin-inexact): New option.
>>> * doc/invoke.texi (-fno-fp-int-builtin-inexact): Document.
>>> * doc/md.texi (floor@var{m}2, btrunc@var{m}2, round@var{m}2)
>>> (ceil@var{m}2): Document dependence on this option.
>>> * ipa-inline-transform.c (inline_call): Handle
>>> flag_fp_int_builtin_inexact.
>>> * ipa-inline.c (can_inline_edge_p): Likewise.
>>> * config/i386/i386.md (rintxf2): Do not test
>>> flag_unsafe_math_optimizations.
>>> (rint<mode>2_frndint): New define_insn.
>>> (rint<mode>2): Do not test flag_unsafe_math_optimizations for 387
>>> or !flag_trapping_math for SSE. Just use gen_rint<mode>2_frndint
>>> for 387 instead of extending and truncating.
>>> (frndintxf2_<rounding>): Test flag_fp_int_builtin_inexact ||
>>> !flag_trapping_math instead of flag_unsafe_math_optimizations.
>>> Change to frndint<mode>2_<rounding>.
>>> (frndintxf2_<rounding>_i387): Likewise. Change to
>>> frndint<mode>2_<rounding>_i387.
>>> (<rounding_insn>xf2): Likewise.
>>> (<rounding_insn><mode>2): Test flag_fp_int_builtin_inexact ||
>>> !flag_trapping_math instead of flag_unsafe_math_optimizations for
>>> x87. Test TARGET_ROUND || !flag_trapping_math ||
>>> flag_fp_int_builtin_inexact instead of !flag_trapping_math for
>>> SSE. Use ROUND_NO_EXC in constant operand of
>>> gen_sse4_1_round<mode>2. Just use gen_frndint<mode>2_<rounding>
>>> for 387 instead of extending and truncating.
>>>
>>> gcc/testsuite:
>>> 2016-05-26 Joseph Myers <jos...@codesourcery.com>
>>>
>>> PR target/71276
>>> PR target/71277
>>> * gcc.dg/torture/builtin-fp-int-inexact.c,
>>> gcc.target/i386/387-builtin-fp-int-inexact.c,
>>> gcc.target/i386/387-rint-inline-1.c,
>>> gcc.target/i386/387-rint-inline-2.c,
>>> gcc.target/i386/sse2-builtin-fp-int-inexact.c,
>>> gcc.target/i386/sse2-rint-inline-1.c,
>>> gcc.target/i386/sse2-rint-inline-2.c,
>>> gcc.target/i386/sse4_1-builtin-fp-int-inexact.c,
>>> gcc.target/i386/sse4_1-rint-inline.c: New tests.
>>>
