[Clang][Codegen] Rint always lowered to llvm intrinsic

[HendrikHuebner]

I was looking at this piece of code in the builtin codegen which decides whether we want to turn a call to a math functions (e.g. sqrt, absf) into a call to an intrinsic instead. I noticed that the boolean logic here will actually set GenerateIntrinsic to true when the builtin is marked as ConstIgnoringExceptions but not ConstIgnoringErrnoAndExceptions. From what I can tell this is only the case for rint defined here.

And indeed, even without -ffast-math and with -O0 (and even the optnone attribute), calls to rint always seem to get replaced by calls to the llvm intrinsic: https://godbolt.org/z/hTvEvs4Pq

Is this the desired behavior?

I think in any case, it would be nice to refactor this logic a little, and move it to a separate function. Originally I was looking into it to do just that, so we can also reuse this logic for ClangIR.

[HendrikHuebner]

cc @andykaylor, I saw you were part of similar discussions before

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Hendrik Hübner (HendrikHuebner)

I was looking at this piece of code in the [builtin codegen](https://github.com/llvm/llvm-project/blob/825ebef51ad83492698cd8ac59c12375fc25636b/clang/lib/CodeGen/CGBuiltin.cpp#L2710) which decides whether we want to turn a call to a math functions (e.g. sqrt, absf) into a call to an intrinsic instead. I noticed that the boolean logic here will actually set `GenerateIntrinsic` to true when the builtin is marked as `ConstIgnoringExceptions` but not `ConstIgnoringErrnoAndExceptions`. From what I can tell this is only the case for `rint` [defined here](https://github.com/llvm/llvm-project/blob/825ebef51ad83492698cd8ac59c12375fc25636b/clang/include/clang/Basic/Builtins.td#L4062).

And indeed, even without -ffast-math and with -O0 (and even the optnone attribute), calls to rint always seem to get replaced by calls to the llvm intrinsic: https://godbolt.org/z/hTvEvs4Pq

Is this the desired behavior?

I think in any case, it would be nice to refactor this logic a little, and move it to a separate function. Originally I was looking into it to do just that, so we can also reuse this logic for ClangIR.

[andykaylor]

Yes, I believe that is the desired behavior. The reasoning is that we do ignore exceptions by default (that is, unless FENV_ACCESS is on), so for functions like rint (and apparently only rint) that may raise exceptions but do not set errno, we can use a call to the intrinsic, which is defined to be equivalent to the libm function except for exception and errno behavior. (If FENV_ACCESS is enabled, we will generate a different intrinisc to preserve exception semantics.)

@zahiraam spent a lot of time getting that code correct, and I would advise you not to change the logic in any way when refactoring. It's quite tricky. On the other hand, it would be nice to have that moved into a shared function, precisely because it is so fragile. Perhaps the function could be added to BuiltinInfo with parameters for the options and function attribute settings that must be checked.

[HendrikHuebner]

Okay, thanks for the advice. I opened a PR to move the logic into a separate function in #168198. I still think we can simplify the logic a little without changing the behavior, to make it a little bit more clear whats going on, but I will make it a separate PR.

[zahiraam]

Yes, I believe that is the desired behavior. The reasoning is that we do ignore exceptions by default (that is, unless FENV_ACCESS is on), so for functions like rint (and apparently only rint) that may raise exceptions but do not set errno, we can use a call to the intrinsic, which is defined to be equivalent to the libm function except for exception and errno behavior. (If FENV_ACCESS is enabled, we will generate a different intrinisc to preserve exception semantics.)

@zahiraam spent a lot of time getting that code correct, and I would advise you not to change the logic in any way when refactoring. It's quite tricky. On the other hand, it would be nice to have that moved into a shared function, precisely because it is so fragile. Perhaps the function could be added to BuiltinInfo with parameters for the options and function attribute settings that must be checked.

It was indeed pretty tricky. But it wouldn't hurt to refactor the code.