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

Consider inlining fldmod to allow optimizing if an arg is a Const #30333

Open
NHDaly opened this issue Dec 10, 2018 · 4 comments
Open

Consider inlining fldmod to allow optimizing if an arg is a Const #30333

NHDaly opened this issue Dec 10, 2018 · 4 comments

Comments

@NHDaly
Copy link
Member

NHDaly commented Dec 10, 2018

Opening an issue based on this discussion: JuliaMath/FixedPointDecimals.jl#43 (review)

In our code, fldmod isn't inlining. Where we are calling it, one of the arguments is a static compiler Const, and so the division could be optimized away (into a cheaper multiplication and bitshift -- explanation here: JuliaMath/FixedPointDecimals.jl#43 (comment)), but it isn't able to because the non-inlined fldmod presents a function barrier that prevents the compiler from knowing one of the arguments is a Const.

fldmod is such a simple function, so i'm surprised it wasn't inlining anyway. Would it make sense to mark this as @inline in Base? If you think so, i can send a PR to do it.

@vchuravy
Copy link
Sponsor Member

vchuravy commented Dec 10, 2018

Is this on 1.0 or on 1.X-dev? And do you have a MWE?

fldmod1 is inlined for me on both 1.0 and 1.X-dev (with normal constant values), but only 1.X-dev is doing the constant propagation.

f() = fldmod1(1, 2)

# 1.0.2
julia> @code_typed f()
CodeInfo(
1 1 ─      (Base.checked_sdiv_int)(1, 2)::Int64                 │╻╷╷  fldmod1
  │        (Base.checked_sdiv_int)(1, 2)::Int64                 ││╻╷╷  mod1
  │   %3 = π (1, Int64)                                         ││╻    mod1
  │        (Base.ifelse)(false, 2, %3)::Int64                   │││  
  └──      return (1, 1)                                        │    
) => Tuple{Int64,Int64}

# 1.X-dev
@code_typed f()
CodeInfo(
1 ─     return (1, 1)
) => Tuple{Int64,Int64}

-- edit:
Or did you mean if only one of the arguments is const?

@NHDaly
Copy link
Member Author

NHDaly commented Dec 10, 2018

Yeah only one of the arguments is const.

@NHDaly
Copy link
Member Author

NHDaly commented Dec 10, 2018

We talked in person, maybe the better, broader solution is to change the inlining heuristics to be aware of constant propagation.

So, if inlining might not seem worth it on its face, but it allows you to propagate a Const you couldn't do otherwise, that should increase the likelihood of inlining by some degree.


Based on some preliminary exploration, maybe that change could be something like adding a check into this function:

function inline_worthy(body::Array{Any,1}, src::CodeInfo, spvals::SimpleVector, slottypes::Vector{Any},
params::Params, cost_threshold::Integer=params.inline_cost_threshold)

that if any of the arguments to this are Core.Compiler.Consts, maybe you should increase the cost_threshold by some amount?

@JeffreySarnoff
Copy link
Contributor

divrem should be included in this (if it too is not being inlined with const arg[s].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants