LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 49179 - Missing copysign(1., x) < 0. to std::signbit(x) fold
Summary: Missing copysign(1., x) < 0. to std::signbit(x) fold
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Scalar Optimizations (show other bugs)
Version: trunk
Hardware: All All
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-14 12:12 PST by Nikita Kniazev
Modified: 2021-04-05 07:17 PDT (History)
4 users (show)

See Also:
Fixed By Commit(s): 85294703a

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Kniazev 2021-02-14 12:12:53 PST 
#include <cmath>

bool foo(double x)
{
    return std::copysign(1., x) < 0.;
}

bool bar(double x)
{
    return std::signbit(x);
}

https://godbolt.org/z/xPMzj9
Comment 1 Craig Topper 2021-02-14 12:17:23 PST 
I'm not sure those are equivalent if x is nan.
Comment 2 Craig Topper 2021-02-14 12:18:28 PST 
Err nevermind I guess they would be.
Comment 3 Sanjay Patel 2021-02-16 13:18:13 PST 
Not sure if Alive2 understands copysign (the LLVM instance seems to be dead currently).

As IR, we have:

define i1 @PR49179(double %x) {
  %s = tail call double @llvm.copysign.f64(double 1.0, double %x)
  %r = fcmp olt double %s, 0.0
  ret i1 %r
}


And we want to turn it into:

  %2 = bitcast double %0 to i64
  %3 = icmp slt i64 %2, 0

So to generalize (this pattern was found in source code in the wild?): "1.0" can be any non-zero number and "0.0" could also be "-0.0"?
Comment 5 Sanjay Patel 2021-02-17 05:30:01 PST 
(In reply to Nikita Kniazev from comment #4)
> > this pattern was found in source code in the wild?
> 
> https://gitlab.gnome.org/GNOME/glib/-/blob/2.64.1/glib/gnulib/signbitd.c#L46
> https://github.com/boostorg/core/blob/
> ddbaa242a99f62eda6aa9db1e3abc0c4bc36134f/include/boost/core/cmath.hpp#L70
> https://stackoverflow.com/a/1905142

Thanks for the links! So the "1.0" and "0.0" are the important special-cases to handle. We probably don't need to fully generalize (it would get complicated if we try to handle zeros/NaN as the copysign argument, arbitrary compare predicates, and arbitrary compare constants).
Comment 6 Sanjay Patel 2021-02-17 07:56:56 PST 
Should be fixed in IR with:
https://reviews.llvm.org/rG85294703a74a

We have seen complications with these kinds of transforms because LLVM supports non-IEEE754-compliant targets, but I think this is always ok. If not, we'll have to move the transform to the backend.

As noted in the comments, if variations of the specified pattern are found, please file other bugs.

And finally (it's not clear from the description if we care about a particular target), if the performance is still not optimal, we may need to adjust codegen too. For x86, I'm seeing something like this currently:

	movmskpd	%xmm0, %eax
	andl	$1, %eax