Re: [PATCH] Reduce floating-point difficulties in timevar.cc

[Matthew Malcomson <matthew.malcomson@arm.com>]

Responding to two emails at the same time ;-)

On 7/21/23 13:47, Richard Biener wrote:
> On Fri, 21 Jul 2023, Matthew Malcomson wrote:
> 
>> On some AArch64 bootstrapped builds, we were getting a flaky test
>> because the floating point operations in `get_time` were being fused
>> with the floating point operations in `timevar_accumulate`.
>>
>> This meant that the rounding behaviour of our multiplication with
>> `ticks_to_msec` was different when used in `timer::start` and when
>> performed in `timer::stop`.  These extra inaccuracies led to the
>> testcase `g++.dg/ext/timevar1.C` being flaky on some hardware.
>>
>> This change ensures those operations are not fused and hence stops the test
>> being flaky on that particular machine.  There is no expected change in the
>> generated code.
>> Bootstrap & regtest on AArch64 passes with no regressions.
> 
> I think this is undesriable.  With fused you mean we use FMA?
> I think you could use -ffp-contract=off for the TU instead.

Yeah -- we used fused multiply subtract because we combined the multiply 
in `get_time` with the subtract in `timevar_accumulate`.

> 
> Note you can't use __attribute__((noinline)) literally since the
> host compiler might not support this.
> 
> Richard.
> 

On 7/21/23 13:49, Xi Ruoyao wrote:
...
> I don't think it's correct.  It will break bootstrapping GCC from other
> ISO C++11 compilers, you need to at least guard it with #ifdef __GNUC__.
> And IMO it's just hiding the real problem.
> 
> We need more info of the "particular machine".  Is this a hardware bug
> (i.e. the machine violates the AArch64 spec) or a GCC code generation
> issue?  Or should we generally use -ffp-contract=off in BOOT_CFLAGS?
> 

My understanding is that this is not a hardware bug and that it's 
specified that rounding does not happen on the multiply "sub-part" in 
`FNMSUB`, but rounding happens on the `FMUL` that generates some input 
to it.

I was given to understand from discussions with others that this codegen 
is allowed -- though I honestly didn't confirm the line of reasoning 
through all the relevant standards.


------------------------
W.r.t. both:
Thanks for pointing out bootstrapping from other ISO C++ compilers -- 
(didn't realise that was a concern).

I can look into `-ffp-contract=off` as you both have recommended.
One question -- if we have concerns that the host compiler may not be 
able to handle `attribute((noinline))` would we also be concerned that 
this flag may not be supported?
(Or is the severity of lack of support sufficiently different in the two 
cases that this is fine -- i.e. not compile vs may trigger floating 
point rounding inaccuracies?)