[SelectionDAG] Handle roundeven libcall in visitCall

[valadaptive]

We can now lower it to its proper instruction.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

Author: None (valadaptive)

Changes

We can now lower it to its proper instruction.

---

Full diff: https://github.com/llvm/llvm-project/pull/170690.diff

1 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+6)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 09a0673bfe1bb..6f22730c90efe 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -9710,6 +9710,12 @@ void SelectionDAGBuilder::visitCall(const CallInst &I) {
         if (visitUnaryFloatCall(I, ISD::FROUND))
           return;
         break;
+      case LibFunc_roundeven:
+      case LibFunc_roundevenf:
+      case LibFunc_roundevenl:
+        if (visitUnaryFloatCall(I, ISD::FROUNDEVEN))
+          return;
+        break;
       case LibFunc_trunc:
       case LibFunc_truncf:
       case LibFunc_truncl:

[arsenm]

Missing tests, but is there any real reason to do this? You can just use the intrinsic? We currently have the worst of all worlds where clang, SimplifyLibCalls, and SelectionDAGBuilder partially handle a different subset of libcalls and intrinsics

[nikic]

I think it's reasonable to do the libcall -> intrinsic replacement somewhere in LLVM, but I agree that that place isn't SDAG. This is something that should be done by SLC and SLC only.

[valadaptive]

I noticed this when working on #170018 (see these tests). It looks like roundeven is already handled in SLC, but not getting converted to an intrinsic in those regression tests. I'm not sure if I just need to enable more optimization passes in the tests or something, but supporting it in SelectionDAG (and hasOptimizedCodegen) seems to be necessary.

[valadaptive]

SimplifyLibCalls is a huge mess right now. There are some functions that try to optimize a libcall and return the corresponding intrinsic as a last resort, there are some that try to optimize a libcall and just bail out entirely if the optimization doesn't work, and there are some (like optimizeSymmetricCall) that optimize to other libcalls and then exit early because, well, the optimization worked.

[arsenm]

SimplifyLibCalls is a huge mess right now.

Correct

There are some functions that try to optimize a libcall and return the corresponding intrinsic as a last resort, there are some that try to optimize a libcall and just bail out entirely if the optimization doesn't work, and there are some (like optimizeSymmetricCall) that optimize to other libcalls and then exit early because, well, the optimization worked.

With the status quo, it's generally works out to do intrinsic->intrinsic transformations, or libcall->libcall transformations. In theory the intrinsic should always works, in practice many fail depending on target library support but there's no formal relationship between the two, or way to query if that will work. TargetLibraryInfo is used as a hack for some intrinsic transforms, but that's not strictly correct

[valadaptive]

In theory the intrinsic should always works, in practice many fail depending on target library support but there's no formal relationship between the two, or way to query if that will work.

Is there any plan for fixing this? I saw you opened a bunch of libcall-related PRs, and I'm wondering if those are working towards improving the situation. I believe the lack of guarantees about what floating-point operations are supported is why Rust can't use roundeven.

Why is it that transforming a libcall into an intrinsic could be incorrect, though? I'd think that if the intrinsic has the same semantics (doesn't set errno, etc), the worst that can happen is that the intrinsic is lowered back into the same libcall. That shouldn't introduce any new compatibility issues.

[arsenm]

Is there any plan for fixing this? I saw you opened a bunch of libcall-related PRs, and I'm wondering if those are working towards improving the situation.

It won't help with this particular issue, but what I'm working towards is supporting emitting libcalls that to functions defined in the module. That will permit handling most intrinsics in the GPU case which fail today.

Why is it that transforming a libcall into an intrinsic could be incorrect, though?

It's not incorrect per se, as it is there's a QoI issue where the intrinsic may fail depending on the target. Trivial cases like roundeven -> llvm.roundeven probably will workout (provided RuntimeLibcallsInfo and TargetLibraryInfo are in agreement). Cases where the legalizer only has the option of calling the host libcall are a liability.

the worst that can happen is that the intrinsic is lowered back into the same libcall.

This assumes you can emit a libcall, and it's not always as simple as a 1:1 mapping. The transforms that have run into issues are where a libcall is transformed into an intrinsic for a different operation, which has worse support on the target. e.g. powf->llvm.ldexp.f32. On windows there is no ldexpf so it becomes a special case that requires additional backend consideration (this case works, but didn't always). There are a number of legalization holes of that shape. The other issue is backend introduced function calls are hidden from the linker, so if you are doing LTO with libm functions defined in the same program, they may have been dropped from the link if they were originally unused. Currently RuntimeLibcallsInfo is used in LTO to conservatively retain functions which may be called, but that does not yet accurately track all functions that the compiler could emit.

I really don't like all of the intrinsics we have that exist solely to call into a host library functions. If the compiler is not providing the functionality (or at least compiler-rt), we probably shouldn't have an intrinsic for it.

[nikic]

I put up #171114 to pursue the reverse direction of this PR.

[valadaptive]

With #171288 fully committing to the opposite direction, this can be closed. Not sure about #170696; hasOptimizedCodegen is called in a few different places besides SelectionDAG.