Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changing the floating point environment interacts badly with interprocedural optimizations #48669

Open
zygoloid mannequin opened this issue Feb 23, 2021 · 1 comment
Open
Labels
bugzilla Issues migrated from bugzilla ipo Interprocedural optimizations

Comments

@zygoloid
Copy link
Mannequin

zygoloid mannequin commented Feb 23, 2021

Bugzilla Link 49325
Version trunk
OS All
Blocks #48670

Extended Description

Testcase:

https://godbolt.org/z/E8Kje7

This example is miscompiled by reordering the call to f(j, a) between two fesetround calls. There are at least two different bugs here (for the testcase with inlining disabled and the testcase with inlining enabled):

  1. With inlining disabled, the call to function f is reordered past a call to function fesetround. 'f' is attributed as 'readnone' (plus 'willreturn nounwind'), so this transform superficially appears correct, but is not: we can't reorder a function that reads the FP environment past a function that changes the FP environment (at least, not within a 'strictfp' function). It's not clear to me whether it's wrong to mark 'f' as 'readnone', or whether we should track the FP environment separately from reads of memory, but I suspect the latter might be a better choice.

  2. With inlining enabled, the inliner adds an 'fadd' instruction to 'g'. That's wrong: 'fadd' can be freely reordered past function calls, but we certainly don't want to allow 'fadd' to be reordered past a call to 'fesetround'. I think the right approach here is that the inliner should produce constrained FP intrinsic calls (assuming the default rounding mode) when inlining into a 'strictfp' function.

As a complementary problem to (2), we also have:

  1. Inlining a constrained FP intrinsic call into a non-'strictfp' function should convert the constrained FP intrinsic call into an unconstrained instruction. This is important because we need to emit the libc++ wrapper functions around C math functions as 'strictfp', and don't want that to be a pessimization.

Right now:

#include
#pragma STDC FENV_ACCESS ON
float f() { return std::rint(10.5); }

... produces wrong results with Clang+libc++ (and also with Clang+libstdc++), because std::rint is built with FENV_ACCESS OFF, so its call to rint is treated as unconstrained. We could fix that by using FENV_ACCESS ON in libc++'s <math.h> wrapper, but that will pessimize all FENV_ACCESS OFF calls unless the inliner is able to undo that when inlining into non-strictfp functions.

@zygoloid
Copy link
Mannequin Author

zygoloid mannequin commented Nov 27, 2021

mentioned in issue #48670

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla ipo Interprocedural optimizations
Projects
None yet
Development

No branches or pull requests

0 participants