refactor inference.jl into multiple files for readability/maintainability #25517

jrevels · 2018-01-11T21:12:32Z

~~I would be flabbergasted if this PR passes CI as it is now.~~ Tests now pass locally.

This PR does a bulk-refactor of inference.jl in order to make the code more readable/maintainable/hackable. One of my long-term motivations here is the development a well-versioned/defined "compiler standard library" - untangling what already exists is the first step.

Here's the summary of the changes:

the contents of coreimg.jl, inference.jl and codevalidation.jl now live in base/compiler/
Core.Inference --> Core.Compiler, and Core.InferenceParams --> Core.Compiler.Params, since these refer to more than just type inference at this point
Constants/structs now mostly appear in the file in which they are first used. Non-business logic methods acting on freshly-defined structs are now defined alongside the structs themselves. Non-business logic methods which don't depend on new structs are now in utilities.jl.
All non-alias const names are now all-caps. Type alias consts are camelcase, and function alias consts are lowercase.
Inference logic is split into several files (abstractinterpretation.jl, typelattice.jl, typeinf.jl, etc.)
All the tfunc definitions now reside in their own file.
Optimization logic (inlining, DCE, etc.) hasn't been broken up at all yet; it is now in optimize.jl. A future PR will be required to break it up.
No semantic changes, at least not on purpose.

I apologize for not breaking things up into smaller commits...it would've taken forever to do this "properly". May God have mercy on the reviewers' souls.

I also really hope that I made this at the right time...rebasing this against any other non-trivial inference PR would probably break my mind.

JeffBezanson · 2018-01-11T21:23:21Z

base/compiler/structs.jl

+# Optimization* #
+#################
+
+mutable struct OptimizationState


Should go with the optimization code.

JeffBezanson · 2018-01-11T21:23:53Z

base/compiler/structs.jl

+# This struct contains information about a use of certain value (`SSAValue` or `Slot`)
+# This might be a toplevel use for `Slot` in which case the `expr` field is `#undef`,
+# and it only happens if the slot might be used before it's defined.
+struct ValueUse


Also part of optimization passes.

JeffBezanson · 2018-01-11T22:57:59Z

base/compiler/init.jl

+# CompilationResult #
+#####################
+
+mutable struct CompilationResult


The name of this should be reverted. It doesn't give you compiled code, just inferred code. Also only used by inference, so it can go in that file.

jrevels · 2018-01-12T05:18:06Z

This successfully builds locally now.

Haven't run the tests yet; I'll let CI run overnight and see how it fares in the morning.

jrevels · 2018-01-12T15:57:27Z

Tests just passed locally! Just waiting on CI now, and any additional review.

StefanKarpinski · 2018-01-12T18:19:50Z

AppVeyor failures are both timeouts. Want me to restart them? Or do we expect this just to timeout again?

jrevels · 2018-01-12T18:29:05Z

Want me to restart them? Or do we expect this just to timeout again?

Are AppVeyor timeouts common these days? I have no reason to suspect that this PR to incur AppVeyor timeouts moreso than any other PR, so if you think it's sensible to try again, then sure, thanks!

StefanKarpinski · 2018-01-12T19:19:17Z

Well, you're looking good on Linux and FreeBSD. Let's see how macOS and Windows do...

JeffBezanson · 2018-01-12T22:03:01Z

👍 Awesome, this is a huge improvement a long time coming.

A few detail comments:

Const and Conditional are also lattice elements so should go in typelattice next to PartialTypeVar
The stuff in typelimits above the comment # limitation parameters # is not really related; maybe move to typeinfer? Or a new file inferencestate.jl?
I think tfuncs.jl should contain only t-functions; there are a bunch of unrelated definitions at the top.
Move return_type_tfunc to tfuncs
exprtype should go in utilities
return_type should also go somewhere else, maybe utilities

jrevels · 2018-01-15T10:29:28Z

Const and Conditional are also lattice elements so should go in typelattice next to PartialTypeVar

The stuff in typelimits above the comment # limitation parameters # is not really related; maybe move to typeinfer? Or a new file inferencestate.jl?

I think tfuncs.jl should contain only t-functions; there are a bunch of unrelated definitions at the top.

Move return_type_tfunc to tfuncs

Done!

exprtype should go in utilities

Done! Also, there's more work to do splitting out code from optimize.jl, but I'm mainly leaving that to a future PR.

return_type should also go somewhere else, maybe utilities

It seems that promotion.jl expects return_type to exist, hence the stub definition before loading Base code. I've moved that stub to directly before include("promotion.jl") and added a comment to make things clearer.

Now, I've just got to get through a rebase and hope CI stays green, and this should be ready to go.

EDIT: rebase complete

EDIT 2: tests pass locally on rebased branch

…lity - the contents of `coreimg.jl`, `inference.jl` and `codevalidation.jl` now live in `base/compiler/` - `Core.Inference` --> `Core.Compiler`, and `Core.InferenceParams` --> `Core.Compiler.Params`, since these refer to more than just type inference at this point - Constants/structs now mostly appear in the file in which they are first used. Non-business logic methods acting on freshly-defined structs are now defined alongside the structs themselves. Non-business logic methods which don't depend on new structs are now in `utilities.jl`. - All non-alias `const` names are now all-caps. Type alias `const`s are camelcase, and function alias `const`s are lowercase. - Inference logic is split into several files (`abstractinterpretation.jl`, `typelattice.jl`, `typeinf.jl`, etc.) - All the tfunc definitions now reside in their own file. - Optimization logic (inlining, DCE, etc.) hasn't been broken up at all yet; it is now in `optimize.jl`. A future PR will be required to break it up. - No semantic changes, at least not on purpose.

jrevels · 2018-01-15T21:45:28Z

Okay, 64-bit Linux failed on Travis and both 32-bit and 64-bit Windows failed on AppVeyor. None of these look PR-related - Travis failed on apt-get install because it couldn't download some dependency, and both Windows failures seems to be due to missing shared object files. I restarted the relevant Travis build, hopefully I'm less unlucky this time.

Could an AppVeyor wizard restart the AppVeyor builds? Thanks!

jrevels · 2018-01-16T15:47:04Z

Looks like a kind soul retriggered my AppVeyor build - 64-bit Windows passed and 32-bit simply timed out. I was also informed that I have the power to retrigger AppVeyor myself, so I've just done so. Fingers crossed for all green this time...

JeffBezanson · 2018-01-16T17:33:31Z

Very nice. I'll merge this now, and Jameson, Keno, and I can rebase around it.

ararslan requested a review from JeffBezanson January 11, 2018 21:18

ararslan added the inference Type inference label Jan 11, 2018

JeffBezanson reviewed Jan 11, 2018

View reviewed changes

jrevels changed the title ~~WIP: rebasing this might make me cry~~ rebasing this might make me cry Jan 12, 2018

jrevels force-pushed the jr/damnthetorpedoes branch from 8b38c4c to ca91862 Compare January 12, 2018 00:57

jrevels changed the title ~~rebasing this might make me cry~~ refactor inference.jl into multiple files for readability/maintainability Jan 12, 2018

jrevels force-pushed the jr/damnthetorpedoes branch 8 times, most recently from 3267296 to f610a64 Compare January 12, 2018 05:15

jrevels force-pushed the jr/damnthetorpedoes branch 3 times, most recently from 2e219d7 to 69015e6 Compare January 12, 2018 15:56

jrevels force-pushed the jr/damnthetorpedoes branch 2 times, most recently from b6822ce to 396ffe7 Compare January 15, 2018 10:26

jrevels force-pushed the jr/damnthetorpedoes branch from 396ffe7 to 7113490 Compare January 15, 2018 10:45

vtjnash approved these changes Jan 16, 2018

View reviewed changes

JeffBezanson approved these changes Jan 16, 2018

View reviewed changes

JeffBezanson merged commit aea9155 into master Jan 16, 2018

JeffBezanson deleted the jr/damnthetorpedoes branch January 16, 2018 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor inference.jl into multiple files for readability/maintainability #25517

refactor inference.jl into multiple files for readability/maintainability #25517

jrevels commented Jan 11, 2018 •

edited

JeffBezanson Jan 11, 2018

JeffBezanson Jan 11, 2018

JeffBezanson Jan 11, 2018

jrevels commented Jan 12, 2018

jrevels commented Jan 12, 2018

StefanKarpinski commented Jan 12, 2018

jrevels commented Jan 12, 2018

StefanKarpinski commented Jan 12, 2018

JeffBezanson commented Jan 12, 2018

jrevels commented Jan 15, 2018 •

edited

jrevels commented Jan 15, 2018

jrevels commented Jan 16, 2018

JeffBezanson commented Jan 16, 2018

refactor inference.jl into multiple files for readability/maintainability #25517

refactor inference.jl into multiple files for readability/maintainability #25517

Conversation

jrevels commented Jan 11, 2018 • edited

JeffBezanson Jan 11, 2018

Choose a reason for hiding this comment

JeffBezanson Jan 11, 2018

Choose a reason for hiding this comment

JeffBezanson Jan 11, 2018

Choose a reason for hiding this comment

jrevels commented Jan 12, 2018

jrevels commented Jan 12, 2018

StefanKarpinski commented Jan 12, 2018

jrevels commented Jan 12, 2018

StefanKarpinski commented Jan 12, 2018

JeffBezanson commented Jan 12, 2018

jrevels commented Jan 15, 2018 • edited

jrevels commented Jan 15, 2018

jrevels commented Jan 16, 2018

JeffBezanson commented Jan 16, 2018

jrevels commented Jan 11, 2018 •

edited

jrevels commented Jan 15, 2018 •

edited