Skip to content

[SimplifyLibCalls] fdim constant fold #109235

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 10, 2024
Merged

[SimplifyLibCalls] fdim constant fold #109235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
672ab25
[SimplifyLibCalls] Constant fold `fdim`
braw-lee Sep 15, 2024
1987c69
[SimplifyLibCalls] Added test for lib calls on win
braw-lee Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions llvm/include/llvm/Transforms/Utils/SimplifyLibCalls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ class LibCallSimplifier {
Value *optimizeTrigInversionPairs(CallInst *CI, IRBuilderBase &B);
Value *optimizeSymmetric(CallInst *CI, LibFunc Func, IRBuilderBase &B);
Value *optimizeRemquo(CallInst *CI, IRBuilderBase &B);
Value *optimizeFdim(CallInst *CI, IRBuilderBase &B);
// Wrapper for all floating point library call optimizations
Value *optimizeFloatingPointLibCall(CallInst *CI, LibFunc Func,
IRBuilderBase &B);
Expand Down
31 changes: 31 additions & 0 deletions llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3109,6 +3109,33 @@ Value *LibCallSimplifier::optimizeRemquo(CallInst *CI, IRBuilderBase &B) {
return ConstantFP::get(CI->getType(), Rem);
}

/// Constant folds fdim
Value *LibCallSimplifier::optimizeFdim(CallInst *CI, IRBuilderBase &B) {
// Cannot perform the fold unless the call has attribute memory(none)
if (!CI->doesNotAccessMemory())
return nullptr;

// TODO : Handle undef values
// Propagate poison if any
if (isa<PoisonValue>(CI->getArgOperand(0)))
return CI->getArgOperand(0);
if (isa<PoisonValue>(CI->getArgOperand(1)))
return CI->getArgOperand(1);

const APFloat *X, *Y;
// Check if both values are constants
if (!match(CI->getArgOperand(0), m_APFloat(X)) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to check the constants if you are doing the general expansion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is just a constant fold right?
also APFloat* is used later to return NaN values, check status of fsub operation
those wouldn't be possible with Value*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, sry, this PR is taking too long, I am new to LLVM and the codebase is too big ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem!

You didn't directly implement this as constant folding. It's almost a general case replacement of the call with a short sequence. A pure constant fold would be done entirely with APFloat, rather than attempting to create intermediate instructions.

It's probably a good idea to leave the general case for a follow up PR, since there are cost considerations (more targets have maxnum-like operations), and last I checked maximum was poorly supported by most backends.

!match(CI->getArgOperand(1), m_APFloat(Y)))
return nullptr;
Comment on lines +3126 to +3129
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't do anything with the constant values, besides check if they are nan (which isn't strong enough to do this transform)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what you mean by that,
we need the NaN check and i don't see if we can do that through a Value*, we need to transform it to constant
can you suggest any alternatives here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to know a dynamic value cannot be nan, which isKnownNeverNaN can do


APFloat Difference = *X;
Difference.subtract(*Y, RoundingMode::NearestTiesToEven);

APFloat MaxVal =
maximum(Difference, APFloat::getZero(CI->getType()->getFltSemantics()));
return ConstantFP::get(CI->getType(), MaxVal);
}

//===----------------------------------------------------------------------===//
// Integer Library Call Optimizations
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -4042,6 +4069,10 @@ Value *LibCallSimplifier::optimizeFloatingPointLibCall(CallInst *CI,
if (hasFloatVersion(M, CI->getCalledFunction()->getName()))
return optimizeBinaryDoubleFP(CI, Builder, TLI);
return nullptr;
case LibFunc_fdim:
case LibFunc_fdimf:
case LibFunc_fdiml:
return optimizeFdim(CI, Builder);
case LibFunc_fminf:
case LibFunc_fmin:
case LibFunc_fminl:
Expand Down
139 changes: 139 additions & 0 deletions llvm/test/Transforms/InstCombine/fdim.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt < %s -passes=instcombine -S | FileCheck %s

define double @fdim_double() {
; CHECK-LABEL: define double @fdim_double() {
; CHECK-NEXT: ret double 2.500000e+00
;
%dim = call double @fdim(double 10.5, double 8.0)
ret double %dim
}

define double @fdim_double1() {
; CHECK-LABEL: define double @fdim_double1() {
; CHECK-NEXT: ret double 0.000000e+00
;
%dim = call double @fdim(double 7.0, double 8.0)
ret double %dim
}

define float @fdim_float() {
; CHECK-LABEL: define float @fdim_float() {
; CHECK-NEXT: ret float 0.000000e+00
;
%dim = call float @fdimf(float 1.500000e+00, float 8.0)
ret float %dim
}

define float @fdim_float1() {
; CHECK-LABEL: define float @fdim_float1() {
; CHECK-NEXT: ret float 2.000000e+00
;
%dim = call float @fdimf(float 1.000000e+01, float 8.0)
ret float %dim
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests for all the edge cases. Can you also test poison/undef arguments in each position

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test,
let me know if there are still some edge cases to be covered

define double @fdim_poison1() {
; CHECK-LABEL: define double @fdim_poison1() {
; CHECK-NEXT: ret double poison
;
%dim = call double @fdim(double poison, double 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fold to poison

Copy link
Contributor Author

@braw-lee braw-lee Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried using maximum intrinsic, it folds
fdim(poison, 1) -> zero
fdim(undef, 1) -> 0x7FF8000000000000
fdim(poison, poison) -> zero
fdim(undef, undef) -> zero

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fdim and maximum should propagate poison, something is missing somewhere

ret double %dim
}

define double @fdim_poison2() {
; CHECK-LABEL: define double @fdim_poison2() {
; CHECK-NEXT: ret double poison
;
%dim = call double @fdim(double 1.0, double poison)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fold to poison

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_APFloat() would not match against undef/poison values
so i used m_Undef() to find any undef/poison values, and returned them

ret double %dim
}

define double @fdim_poison3() {
; CHECK-LABEL: define double @fdim_poison3() {
; CHECK-NEXT: ret double poison
;
%dim = call double @fdim(double poison, double poison)
ret double %dim
}

; undef folding is not implemented yet
define double @fdim_undef1() {
; CHECK-LABEL: define double @fdim_undef1() {
; CHECK-NEXT: [[DIM:%.*]] = call double @fdim(double undef, double 1.000000e+00)
; CHECK-NEXT: ret double [[DIM]]
;
%dim = call double @fdim(double undef, double 1.0)
ret double %dim
}

define double @fdim_inf_ninf() {
; CHECK-LABEL: define double @fdim_inf_ninf() {
; CHECK-NEXT: ret double 0x7FF0000000000000
;
%dim = call double @fdim(double 0x7FF0000000000000, double 0x8000000000000000 )
ret double %dim
}

define double @fdim_inf() {
; CHECK-LABEL: define double @fdim_inf() {
; CHECK-NEXT: ret double 0x7FF8000000000000
;
%dim = call double @fdim(double 0x7FF0000000000000, double 0x7FF0000000000000)
ret double %dim
}

define double @fdim_nzero() {
; CHECK-LABEL: define double @fdim_nzero() {
; CHECK-NEXT: ret double 0.000000e+00
;
%dim = call double @fdim(double -0.0, double +0.0)
ret double %dim
}

define double @fdim_strictfp() {
; CHECK-LABEL: define double @fdim_strictfp() {
; CHECK-NEXT: [[DIM:%.*]] = call double @fdim(double 1.000000e+01, double 8.000000e+00) #[[ATTR1:[0-9]+]]
; CHECK-NEXT: ret double [[DIM]]
;
%dim = call double @fdim(double 10.0, double 8.0) strictfp
ret double %dim
}

define double @fdim_nan1() {
; CHECK-LABEL: define double @fdim_nan1() {
; CHECK-NEXT: ret double 0x7FF8000000000000
;
%dim = call double @fdim(double 10.0, double 0x7FF8000000000000)
ret double %dim
}


define double @fdim_nan2() {
; CHECK-LABEL: define double @fdim_nan2() {
; CHECK-NEXT: ret double 0x7FF8000000000000
;
%dim = call double @fdim(double 0x7FF8000000000000, double 1.4)
ret double %dim
}

define double @fdim_snan1() {
; CHECK-LABEL: define double @fdim_snan1() {
; CHECK-NEXT: ret double 0x7FFC000000000000
;
%dim = call double @fdim(double 0x7FF4000000000000, double 1.4)
ret double %dim
}

define double @fdim_snan2() {
; CHECK-LABEL: define double @fdim_snan2() {
; CHECK-NEXT: ret double 0x7FFC000000000000
;
%dim = call double @fdim(double 1.7, double 0x7FF4000000000000)
ret double %dim
}

declare double @fdim(double, double) #0
declare float @fdimf(float, float) #0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some strictfp tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lib calls don't fold for strictfp cases, added test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the nan tests are still missing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added nan tests
0x7FF8000000000000 is the NAN value right?

Copy link
Contributor

@arsenm arsenm Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a (quiet) nan value. If you want to be very comprehensive you can check payloads and snan quieting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added tests for snan
let me know if anything else is needed

attributes #0 = { memory(none) }
36 changes: 36 additions & 0 deletions llvm/test/Transforms/InstCombine/win-fdim.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
; RUN: opt %s -passes=instcombine -S -mtriple=i386-pc-windows-msvc | FileCheck %s --check-prefixes=MSVC19

define double @fdim_double() {
; MSVC19-LABEL: define double @fdim_double() {
; MSVC19-NEXT: ret double 2.500000e+00
;
%dim = call double @fdim(double 10.5, double 8.0)
ret double %dim
}

;fdimf is not supported by windows
define float @fdim_float() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arsenm
this test should fail because windows should not be able to fold @fdimf, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the call should remain

; MSVC19-LABEL: define float @fdim_float() {
; MSVC19-NEXT: [[DIM:%.*]] = call float @fdimf(float 1.500000e+00, float 8.000000e+00)
; MSVC19-NEXT: ret float [[DIM]]
;
%dim = call float @fdimf(float 1.500000e+00, float 8.0)
ret float %dim
}

;fdiml is not supported by windows
define fp128 @fdim_long() {
; MSVC19-LABEL: define fp128 @fdim_long() {
; MSVC19-NEXT: [[DIM:%.*]] = call fp128 @fdiml(fp128 0xL00000000000000000000000000000000, fp128 0xL00000000000000000000000000000000)
; MSVC19-NEXT: ret fp128 [[DIM]]
;
%dim = call fp128 @fdiml(fp128 0xL00000000000000000000000000000000 , fp128 0xL00000000000000000000000000000000)
ret fp128 %dim
}

declare double @fdim(double, double) #0
declare float @fdimf(float, float) #0
declare fp128 @fdiml(fp128, fp128) #0

attributes #0 = { memory(none) }
Loading