Skip to content
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.

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

Allow constant-folding intrinsics that are non-pure for inference only #31193

Closed
wants to merge 1 commit into from

Conversation

Keno
Copy link
Member

@Keno Keno commented Feb 27, 2019

Any constants produced during inference must be exact (in the === to
what would have been produced at runtime sense). As a result, we disallow
constant folding the sqrt intrinsic, since we can't guarantee that this
will be the case (depending on what LLVM decides to do). However, this does
not apply to the optimizer (and in fact we do it all the time by inlining
pure functions here). Thus, for code size, inlining heuristics and non-LLVM
backends, enable the optimizer to constant fold sqrt.

Before:

julia> f() = sqrt(2)
f (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 ─ %1 = Base.Math.sqrt_llvm(2.0)::Float64
└──      return %1
) => Float64

julia> @code_typed optimize=false f()
CodeInfo(
1 ─ %1 = Main.sqrt(2)::Float64
└──      return %1
) => Float64

After

julia> @code_typed f()
CodeInfo(
1 ─     return 1.4142135623730951
) => Float64

julia> @code_typed optimize=false f()
CodeInfo(
1 ─ %1 = Main.sqrt(2)::Float64
└──      return %1
) => Float64

Note that we are not able to infer Const, but still inline the
constant in the optimized version of the IR.

@vtjnash
Copy link
Member

vtjnash commented Feb 28, 2019

Yep, this seems like a good step down the road of making a proper @pure interface. I was hoping someone would get to this eventually. All of the _fast intrinsics should be in this list too (of things that can be locally optimized for performance, but are not eligible for inter-proceedural inference). Usually we have just leaned on the knowledge that LLVM already has these optimizations, so this PR is desirable, even though it hasn't been critical.

# invalid intrinsic
return nothing
end
(min, max, _) = T_IFUNC[iidx]
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you're trying to do error checking here, then only get around to doing half of it? I think we can reuse most of pure_eval_call here, rather than implementing an arbitrary subset of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, oops that code got lost somewhere. I don't think pure_eval_call handles intrinsics though.

Copy link
Member

Choose a reason for hiding this comment

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

huh, that seems sort of a little bit silly. I didn't realize we had a third copy of that code sitting around too.

Copy link
Member Author

Choose a reason for hiding this comment

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

So should I add support for intrinsics to pure_eval_call then? I would kind of like to avoid the try/catch since we do have nothrow modeling for intrinsics.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, I've just added the error check here for now.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's probably OK to reuse that support, although a bit less general (can't handle some functions where it might throw, but computing that set is pretty tedious—e.g. sizeof_tfunc). But it doesn't look like you've got any calls to the no-throw predicate right now.

Copy link
Member

Choose a reason for hiding this comment

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

Seems to still be missing a call to the nothrow-test or a try/catch to handle it?

Any constants produced during inference must be *exact* (in the === to
what would have been produced at runtime sense). As a result, we disallow
constant folding the sqrt intrinsic, since we can't guarantee that this
will be the case (depending on what LLVM decides to do). However, this does
not apply to the optimizer (and in fact we do it all the time by inlining
pure functions here). Thus, for code size, inlining heuristics and non-LLVM
backends, enable the optimizer to constant fold sqrt.

Before:
```
julia> f() = sqrt(2)
f (generic function with 1 method)

julia> @code_typed f()
CodeInfo(
1 ─ %1 = Base.Math.sqrt_llvm(2.0)::Float64
└──      return %1
) => Float64

julia> @code_typed optimize=false f()
CodeInfo(
1 ─ %1 = Main.sqrt(2)::Float64
└──      return %1
) => Float64
```

After
```
julia> @code_typed f()
CodeInfo(
1 ─     return 1.4142135623730951
) => Float64

julia> @code_typed optimize=false f()
CodeInfo(
1 ─ %1 = Main.sqrt(2)::Float64
└──      return %1
) => Float64
```

Note that we are not able to infer `Const`, but still inline the
constant in the optimized version of the IR.
@Keno
Copy link
Member Author

Keno commented Feb 28, 2019

Playing with this, an interesting phenomenon that comes us is that other intrinsics in the same function become eligible for constant folding, but since they weren't constant at inference time they don't. I guess that means inlining needs to re-check if all the arguments are constant.

@vtjnash
Copy link
Member

vtjnash commented Feb 28, 2019

Yes, in part that's why I want this to eventually handle as much as the other pure_eval_call paths were also expected to able to handle. I guess we then just need to iterate this pass to convergence? Shouldn't a top-to-bottom pass over the sorted nodes generally already handle most cases though just via SSA value domination?

@kshyatt kshyatt added the compiler:inference Type inference label Feb 28, 2019
@mbauman mbauman added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Feb 28, 2019
f === Intrinsics.cglobal) # cglobal lookup answer changes at runtime
end

# whether `f` is pure for inference
is_pure_intrinsic_optimize_only(f) = f === Intrinsics.sqrt_llvm # this one may differ by a few ulps at runtime
Copy link
Member

Choose a reason for hiding this comment

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

This list should have all of the _fast intrinsics too plus muladd_float. I thought I had remembered to add those to the is_pure_intrinsic_infer list originally, but seems like somehow only sqrt_llvm actually made it on the list—which is arguably the least likely one to actually be a problem.

# invalid intrinsic
return nothing
end
(min, max, _) = T_IFUNC[iidx]
Copy link
Member

Choose a reason for hiding this comment

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

Seems to still be missing a call to the nothrow-test or a try/catch to handle it?

@oscardssmith
Copy link
Member

Can this be resurrected?

@aviatesk aviatesk self-assigned this Mar 30, 2022
aviatesk added a commit that referenced this pull request Jul 22, 2022
We also need to mark `muladd` as IPO-in`:consistent, but it requires to
revive #31193 to preserve the currently available optimizations so left
for a future PR.
aviatesk added a commit that referenced this pull request Jul 23, 2022
We also need to mark `muladd` as IPO-in`:consistent, but it requires to
revive #31193 to preserve the currently available optimizations so left
for a future PR.
aviatesk added a commit that referenced this pull request Jul 23, 2022
We also need to mark `muladd` as not IPO-`:consistent, but it requires
to revive #31193 to preserve the currently available optimizations so
I left it as TODO for now.
aviatesk added a commit that referenced this pull request Aug 5, 2022
We also need to mark `muladd` as not IPO-`:consistent, but it requires
to revive #31193 to preserve the currently available optimizations so
I left it as TODO for now.
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
We also need to mark `muladd` as not IPO-`:consistent, but it requires
to revive JuliaLang#31193 to preserve the currently available optimizations so
I left it as TODO for now.
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
We also need to mark `muladd` as not IPO-`:consistent, but it requires
to revive JuliaLang#31193 to preserve the currently available optimizations so
I left it as TODO for now.
@Keno
Copy link
Member Author

Keno commented Dec 31, 2022

This particular case was fixed in #43786. We also have the effect system now that says things about this more rigorously, though we don't use it yet here. Regardless, this particular change is outdated.

@Keno Keno closed this Dec 31, 2022
@aviatesk aviatesk deleted the kf/sqrt_inline branch August 6, 2023 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants