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

Extend code_typed to be able to debug constant prop #29261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Keno
Copy link
Member

@Keno Keno commented Sep 18, 2018

I'm currently debugging code that uses a lot of arrays with
dimensions in their type parameters. This type of code heavily
relies on constant propagation to lift dimensions from the value
domain into the type domain. Unfortunately, it's a bit hard to
discover what exactly causes inference to drop information from
constants, because there's no way to feed in constants for a
particular invocation using code_typed. This is a quick hack to
remidy that, by making $-interpolated expressions available
as constants to type inference, e.g.

julia> @code_typed 1+1
CodeInfo(
53 1%1 = (Base.add_int)(x, y)::Int64
   └──      return %1
) => Int64

julia> @code_typed $(1)+$(1)
CodeInfo(
53 1return 2
) => Int64

This is missing handling for a bunch of the other cases in code_typed, which is why this is WIP. I plan to put those in and finish this up once I've confirmed this is useful, but figured I'd put it up in the meantime.

@Keno Keno force-pushed the kf/codetypedconst branch 3 times, most recently from 6991561 to ec3c790 Compare September 19, 2018 00:20
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 26, 2018

This capability does seem useful. I'd like to base it on top of #28955 though, so that hopefully the internal API can be better.

@Keno
Copy link
Member Author

Keno commented Sep 26, 2018

Yes, I agree. This also needs to be cleaned up a bit, but I've been using it quite extensively and it's been very useful.

@Keno
Copy link
Member Author

Keno commented Oct 6, 2018

Rebased on top of current master (i.e. including #28955), extended to handle a few extra syntax forms. Additionally, the Symbol argument in get/setproperty! now gets automatically consted (only in the syntax form of course).

@Keno Keno changed the title WIP: Extend code_typed to be able to debug constant prop Extend code_typed to be able to debug constant prop Oct 6, 2018
I'm currently debugging code that uses a lot of arrays with
dimensions in their type parameters. This type of code heavily
relies on constant propagation to lift dimensions from the value
domain into the type domain. Unfortunately, it's a bit hard to
discover what exactly causes inference to drop information from
constants, because there's no way to feed in constants for a
particular invocation using code_typed. This is a quick hack to
remidy that, by making `$`-interpolated expressions available
as constants to type inference, e.g.

```julia
julia> @code_typed 1+1
CodeInfo(
53 1 ─ %1 = (Base.add_int)(x, y)::Int64
   └──      return %1
) => Int64

julia> @code_typed $(1)+$(1)
CodeInfo(
53 1 ─     return 2
) => Int64
```

Additionally, this extends the same mechanism to apply to
get/setproperty!, even in the absence of `$` for the symbol
argument, reflecting their special casing inference.
@Keno
Copy link
Member Author

Keno commented Oct 9, 2018

In any case, this is good to go from my perspective. Any further review comments? Otherwise I'll plan to merge in a day or so.

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I'm not sure I'll have a chance to finish review this week. Can you ping me again next week, unless someone else can make sure the _gen_call_with_extracted_types are correct and won't make it difficult to improve this code in the future (#20159 is the only open issue I can find to reference, so maybe we didn't open / prematurely closed the others)?

for i = 1:length(types.parameters)+1
argtypes[i] = isassigned(constvals, i) ?
Core.Compiler.Const(constvals[i]) : (
i == 1 ? typeof(f) : types.parameters[i-1])
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

There appear to be a couple issues with this (invalid assumptions about the structure of types, typeof instead of Core.Typeof). Would it be possible to format this better too? The deeply nested conditionals and indentation are hard to follow.

@Keno
Copy link
Member Author

Keno commented Oct 17, 2018

@vtjnash ping. Other than the obvious Typeof, could you suggest what the correct way to write this is?

@jrevels
Copy link
Member

jrevels commented Nov 2, 2018

Bump. @vtjnash ping again 🙂

@vchuravy
Copy link
Sponsor Member

Bump. This would be quite useful to have for https://github.com/vchuravy/Cthulhu.jl!

@User-764Q
Copy link

This issue is a few years old now, is it worth keeping it open or can it be closed?

Matt.

@timholy
Copy link
Sponsor Member

timholy commented Sep 26, 2021

Cthulhu is now quite good for debugging constprop, and that seems like a fine place for me. But I'll let @Keno decide whether to close this.

@vchuravy
Copy link
Sponsor Member

I think the question is do we want to expose entering type-inference with const-args on the initial frame?

code_typed((Float64,)) do (x,)
   sin(1.0) + x
end

Is the current "canonical" workaround.
cc: @aviatesk

aviatesk added a commit that referenced this pull request Nov 9, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 9, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 9, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 9, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 10, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 10, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 10, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
aviatesk added a commit that referenced this pull request Nov 10, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
KristofferC pushed a commit that referenced this pull request Nov 13, 2022
This interface is necessary for us to implement IPO-able customized
external lattice implementation.
Additionally `InferenceResult(::MethodInstance, ::SimpleArgtypes)`
constructor may be useful for debugging const-prop' inference or
implement something like #29261.
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

Successfully merging this pull request may close these issues.

None yet

6 participants