llvm / llvm/llvm-project

[InstCombine] miss fabs(sqrt(x)) > C fold for positive C

Open
#201,259 1 comment 0 reactions 1 assignee Claimed by @AnkitDubeycs25 View on GitHub
llvm:instcombine missed-optimization
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

InstCombine does not remove a redundant `fabs` in an ordered comparison against a positive constant when the operand is the result of `sqrt`.

Reduced IR:

```llvm
define i1 @miss(double %x) {
entry:
%s = call double @llvm.sqrt.f64(double %x)
%f = call double @llvm.fabs.f64(double %s)
%c = fcmp ogt double %f, 1.000000e+00
ret i1 %c
}
```
Current behavior (https://godbolt.org/z/WxocWq3jT) keeps the fabs.

Expected:
```llvm ir
define i1 @miss(double %x) {
entry:
%s = call double @llvm.sqrt.f64(double %x)
%c = fcmp ogt double %s, 1.000000e+00
ret i1 %c
}
```
`sqrt()` has three possible FP classes: NaN, -0.0 or +0.0, and positive. All of them arithmetically ignore `fabs()` within a `fcmp` with a positive C.

Online alive2 timed out, so I paste my local running result here:

Image

This is a real pattern observed from SPEC 2017 `511.povray_r`. The original context is like:
```c
inline void VLength(DBL& a, const VECTOR b)
{
a = sqrt(b[X] * b[X] + b[Y] * b[Y] + b[Z] * b[Z]);
}

static DBL normalize(VECTOR A, VECTOR B)
{
DBL tmp;

VLength(tmp, B);

if (fabs(tmp) > EPSILON) // redundant fabs()
{
VInverseScale(A, B, tmp);
}
else
{
Make_Vector(A, 0.0, 1.0, 0.0);
}

return(tmp);
}
```
In its final AArch64 assembly there is exactly the redundant fabs:
```assembly
fsqrt d2, d2
fabs d3, d2
fcmp d3, d15
```

Maybe on `fabs()` we could explicitly query FPClass to remove it based on the data flow facts, rather than relying on a specific pattern.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.