[sil-optimizer] Add FP comparison support in constant folder #69828

jkshtj · 2023-11-13T23:40:47Z

This pull request adds support for handling floating point comparisons to the constant folder.

Partially fixes #68901

jkshtj · 2023-11-13T23:41:24Z

@BradLarson could you please trigger swift-ci?

jkshtj · 2023-11-13T23:41:40Z

@eeckstein could you please take a look at the PR?

asl · 2023-11-14T05:16:51Z

lib/SILOptimizer/Utils/ConstantFolding.cpp

+  bool result;
+  bool isOrdered = !lhs.isNaN() && !rhs.isNaN();
+
+  switch (ID) {


Please ensure your coding style is same as in the file

This is actually git-clang-format. But I can bypass that step to keep the style same.

Well... git-clang-format is dumb and overall the style guide is not simply clang-format everything.

not simply clang-format everything.

Shouldn't it be? What is the benefit of using a formatter then?

It can handle maybe 80% of cases. However, the remaining 20% of cases are about things like readability, etc.

However, in your particular case it's not configured properly. The repo's style config is explicitly saying based on LLVM. And LLVM defaults to 2 spaces indent. So, something is not correct here and this is why one need to examine the output of formatter not merely commit it as-is.

asl · 2023-11-14T05:19:13Z

test/AutoDiff/SILOptimizer/vjp_and_pullback_inlining.swift

@@ -40,7 +40,12 @@ func caller_of_simple_vjp() -> Float {
 @_silgen_name("pb_with_control_flow")
 func pb_with_control_flow(_ x: Float) -> Float {
  if (x > 0) {
-    return sin(x) * cos(x)


I would just ensure you won't have x constant folded here. For example, you could have an argument of caller_of_pb_with_control_flow that you will use to evaluate gradient at.

Ah nice catch. Fixed.

eeckstein

Nice!
Do you mind adding a swift (not sil) test file which compares constant folded results with not-constant folded floating point operations? This would not only test the implementation but also the assumptions about the floating point behavior (like "Ordered comparisons with NaN always return false", etc).

You can do that for example by generating the test file, like e.g. done here: https://github.com/apple/swift/blob/main/validation-test/SILOptimizer/static_enums_fuzzing.swift.
Then it's easy to test all combinations of fp-types * operations * relevant-operand-values

jkshtj · 2023-11-16T22:38:37Z

@eeckstein Added the validation tests as you suggested. They helped catch quite a lot of edge cases so thanks for pointing it out here!

Can you please take a look at the revision when you get time?

eeckstein

Very nice! Thanks!
LGTM

eeckstein · 2023-11-17T07:02:00Z

validation-test/SILOptimizer/constant_folded_fp_operations.swift

+        }
+    }
+
+        func generateComparisonFuncDecls() {


wrong indentation

eeckstein · 2023-11-17T07:12:15Z

validation-test/SILOptimizer/constant_folded_fp_operations.swift

+                        @inline(never) @_silgen_name("\(comparisonFuncName)") @_optimize(none)
+                        func \(comparisonFuncName)() {
+                            let \(resultVarName) = \(optFuncName)() == \(unoptFuncName)()
+                            print("[\(resultVarName)] result equal? \\(\(resultVarName))")


You could just emit a precondition(result, "message") instead of FileChecking the output. But checking the output is also fine

Nice! Changed.

jkshtj · 2023-11-20T16:45:40Z

@eeckstein we may need to restart swift-ci.

asl · 2023-11-20T17:04:49Z

@swift-ci please test

jkshtj · 2023-11-20T18:00:06Z

Looks like a validation test failed. I'm taking a look.

jkshtj · 2023-11-20T19:27:19Z

Fixed the failing validation test.

This commit modifes the constant folder to not handle equality comparisons b/w
infinity and non-infinity operands. In such comparisons, special floating point
types - Float80 and Float16, may come into play and pattern matching againt them
complicates the constant folding logic more than we'd like.

BradLarson · 2023-11-20T22:41:38Z

@swift-ci Please test

jkshtj · 2023-11-27T19:00:04Z

@eeckstein I believe this can be merged?

eeckstein · 2023-11-27T19:03:45Z

Basically yes.
Please rebase + retest it first

This commit modifes the constant folder to not handle equality comparisons b/w infinity and non-infinity operands. In such comparisons, special floating point types - Float80 and Float16, may come into play and pattern matching againt them complicates the constant folding logic more than we'd like.

jkshtj · 2023-11-27T21:10:56Z

@eeckstein Here's my changes rebased on top of trunk. Can you please kick off swift-ci?

eeckstein · 2023-11-27T21:23:48Z

@swift-ci test

jkshtj · 2023-11-28T00:59:25Z

Windows build seems to have timed out.

asl · 2023-11-28T00:59:56Z

@swift-ci please test windows

tbkka · 2024-02-15T18:05:46Z

CC: @stephentyrone

tbkka · 2024-02-15T18:06:37Z

@jkshtj -- Do the LLVM optimizations not handle these cases? Generally, LLVM is pretty good at this sort of thing.

jkshtj · 2024-02-15T19:12:29Z

@jkshtj -- Do the LLVM optimizations not handle these cases? Generally, LLVM is pretty good at this sort of thing.

@tbkka we were motivated to perform FP constant-folding at the SIL level because it vastly simplified the generated derivative code. Below is a minimal example of the optimizations we wanted the FP constant-folding to enable.

@differentiable(reverse)
func f(_ x: Float) -> Float {
  if (x > 0) {
    return sin(x) * cos(x)
  } else {
    return sin(x) + cos(x)
  }
}

// With FP constant-folding the control-flow in VJP of `f` 
// is removed after it's inlined into `foo`, which eventually 
// enables further inlining of all VJPs and pullbacks involved.
//
// Inlining of VJPs and pullbacks lets us get rid of closure context
// allocations, which form the most latent chunk of AD code.
@inline(never)
func foo() -> Float {
  gradient(at: Float(4), of: f(x) )
}

asl reviewed Nov 14, 2023

View reviewed changes

eeckstein reviewed Nov 14, 2023

View reviewed changes

jkshtj force-pushed the main branch from 02c2e04 to 1cb5f57 Compare November 16, 2023 22:34

eeckstein approved these changes Nov 17, 2023

View reviewed changes

jkshtj force-pushed the main branch from 1cb5f57 to 7a2dfde Compare November 17, 2023 17:09

jkshtj added 3 commits November 27, 2023 11:32

[sil-optimizer] Add FP comparison support in constant folder

97a5a83

[sil-optimizer] Add validation tests for constant folded FP arithmetic

c800dd7

jkshtj force-pushed the main branch from 3237ded to cc3c51e Compare November 27, 2023 21:02

eeckstein merged commit 2c43c27 into swiftlang:main Nov 28, 2023

tbkka mentioned this pull request Feb 14, 2024

ConstantFolding: fix wrong constant folding for float-infinity compares #71610

Merged

[sil-optimizer] Add FP comparison support in constant folder #69828

[sil-optimizer] Add FP comparison support in constant folder #69828

Uh oh!

Conversation

jkshtj commented Nov 13, 2023

Uh oh!

jkshtj commented Nov 13, 2023

Uh oh!

jkshtj commented Nov 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asl Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asl Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eeckstein left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkshtj commented Nov 16, 2023

Uh oh!

eeckstein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkshtj commented Nov 20, 2023

Uh oh!

asl commented Nov 20, 2023

Uh oh!

jkshtj commented Nov 20, 2023

Uh oh!

jkshtj commented Nov 20, 2023

Uh oh!

BradLarson commented Nov 20, 2023

Uh oh!

jkshtj commented Nov 27, 2023

Uh oh!

eeckstein commented Nov 27, 2023

Uh oh!

jkshtj commented Nov 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eeckstein commented Nov 27, 2023

Uh oh!

jkshtj commented Nov 28, 2023

Uh oh!

asl commented Nov 28, 2023

Uh oh!

tbkka commented Feb 15, 2024

Uh oh!

tbkka commented Feb 15, 2024

Uh oh!

jkshtj commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

asl Nov 14, 2023 •

edited

Loading

asl Nov 14, 2023 •

edited

Loading

eeckstein left a comment •

edited

Loading

jkshtj commented Nov 27, 2023 •

edited

Loading

jkshtj commented Feb 15, 2024 •

edited

Loading