Skip to content

Missed optimization: dead code is not eliminated based on possible floating-point values at any optimization level. #82381

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

Open
Archaic-Dreams opened this issue Feb 20, 2024 · 9 comments

Comments

@Archaic-Dreams
Copy link

https://godbolt.org/z/6sWTPvne9

#include <algorithm>
#include <stdexcept>

class TestClass
{
public:
    void SetValue(float value);

private:
    float m_Value;
};

void TestClass::SetValue(float value)
{
    if (value >= 0.0f && value <= 100.0f) {
        m_Value = value;
    }
    else {
        throw std::out_of_range("Value must be [0, 100].");
    }
}

void TestFunc(TestClass& t, float value)
{
    value = std::clamp(value, 30.0f, 50.0f);
    // When TestClass::SetValue is inlined, the exception throwing code is not eliminated.
    // Given that at this point we can prove that 'value' lies in the range [30.0f, 50.0f] well within the range required by the setter function, we can rid the not taken paths of code.
    t.SetValue(value);
}

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 20, 2024

Currently we don't have a plan to eliminate fcmps based on ranges.
Related issues: #68301 #70985
See also https://discourse.llvm.org/t/floating-point-range-checks/34923

@Archaic-Dreams
Copy link
Author

Archaic-Dreams commented Feb 20, 2024

I see. I bet adding support like this would take a pretty big effort if the constraint system is designed around integers. However, It seems like something to keep on the table for the future. Bounds-checking situations like the one described above can produce a decent amount of unneeded overhead especially for code that enforces many constraints on values.

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 20, 2024

@nikic What do you think of handling some trivial cases with ValueTracking/InstCombine/DomCondCache? Refactoring ValueLattice for CVP/SCCP seems to require a lot of effort.

Anyway we should implement ConstantRangeFP first.

@dtcxzyw dtcxzyw self-assigned this Feb 20, 2024
@Explorer09
Copy link

Just curious. I might not know the C++ language well, but isn't it dangerous to optimize out that exception throw when the value in TestFunc() could be a NaN?

If I remember correctly, std::clamp(std::nanf(""), 30.0f, 50.0f) return a NaN, doesn't it?

@arsenm
Copy link
Contributor

arsenm commented Feb 26, 2024

Just curious. I might not know the C++ language well, but isn't it dangerous to optimize out that exception throw when the value in TestFunc() could be a NaN?

Floating point exceptions aren't exceptions in the C++ sense. The invalid operation exception would be raised if there was a signaling nan, but you have to enable fenv access for that

@Explorer09
Copy link

@arsenm I was referring to a quiet NaN in a C++ context. Even though comparing with an operator< would set an FP exception (which is different from C++ exception), my point is that it can escape the std::clamp check, and put the NaN in t.SetValue.

@dtcxzyw
Copy link
Member

dtcxzyw commented Feb 26, 2024

@arsenm I was referring to a quiet NaN in a C++ context. Even though comparing with an operator< would set an FP exception (which is different from C++ exception), my point is that it can escape the std::clamp check, and put the NaN in t.SetValue.

We cannot do the optimization if value may be a NaN.

@Explorer09
Copy link

We cannot do the optimization if value may be a NaN.

Update. It seems that the OP used the "-Ofast" option in the godbolt link (https://godbolt.org/z/6sWTPvne9), which implied "-ffast-math" and that we don't need to care about NaN case. But generally the optimization proposed is unsafe, as we need to keep the IEEE behavior regarding NaNs.

@Archaic-Dreams
Copy link
Author

Archaic-Dreams commented Feb 26, 2024

Yes. I did consider the nuances of IEEE floats like this, and nans do cause an issue with such an optimization. I probably should have made that more clear earlier on that only fast math modes would apply for an optimization like this. In fast math mode contexts where IEEE behaviors are not required though, it should still be possible to do something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants