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

turn on linear IR #24113

Merged
merged 4 commits into from
Dec 11, 2017
Merged

turn on linear IR #24113

merged 4 commits into from
Dec 11, 2017

Conversation

JeffBezanson
Copy link
Member

This should be a fairly agreeable version of #24027. All calls are pulled out of argument position, but are still allowed as any assignment RHS and as arguments to return. The Expr .typ field is still there. I updated codevalidation.jl with the rules implemented here, and got it passing on everything. I hacked in a solution for cglobal by pre-evaluating constant tuples in jl_resolve_globals.

This only increases the sysimg by about 15%, and with a few more things like #24109 I think we'll be fine. I think we should merge this soon and work on optimizations later.

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: stored type BenchmarkTools.ParametersPreV006 does not match currently loaded type

Logs and partial data can be found here
cc @ararslan

@ararslan
Copy link
Member

Nanosoldier won't be functional again until JuliaIO/JLD.jl#196 is fixed.

@JeffBezanson
Copy link
Member Author

JeffBezanson commented Oct 13, 2017

Ah, llvmcall has the same problem as cglobal, but more complex. It will probably have to be a macro that expands to a foreigncall (edit: or an llvmcall expr head).

@vtjnash
Copy link
Member

vtjnash commented Nov 7, 2017

If you rebase this, we can now run nanosoldier against it

@JeffBezanson
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@JeffBezanson
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@JeffBezanson
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@StefanKarpinski
Copy link
Member

Oy vey. Some of those regressions!

@quinnj
Copy link
Member

quinnj commented Nov 15, 2017

Looks like it's almost entirely SubArray benchmarks w/ significant regressions though, so at least it's targeted.

@JeffBezanson
Copy link
Member Author

Yep, looks scary but not a huge deal. I've been picking these off one by one.

@vtjnash
Copy link
Member

vtjnash commented Nov 15, 2017

How is the performance of this for building the system image and running tests? On my machine, it seems to be about 25% slower 10% slower at building and generated about 25% larger AST data. But also seems to have emitted about 10% more functions (I'm not sure whether that's good or bad).

sysimg size breakdown (PR):
     sys data: 52669172
  isbits data: 28600958
      symbols:   260314
    tags list:  2679720
   reloc list: 10264512
    gvar list:    76048
    fptr list:   238240

sysimg size breakdown (master):
     sys data: 52659044
  isbits data: 22744512
      symbols:   266667
    tags list:  2589636
   reloc list:  9858840
    gvar list:    76232
    fptr list:   212744

EDIT: fixed build time – I realized I used the wrong reference initially

@JeffBezanson
Copy link
Member Author

Yes, I see the same numbers. I think we'll be able to simplify some of the optimization passes (probably including merging #23240) and add some more compact encodings of common patterns like ssavalue assignment. There are also lots of sequences like this:

        # meta: location subarray.jl reindex 179
        SSAValue(309) = SSAValue(103)
        # meta: pop location

that we can peephole optimize away. We'll see how far that gets us.

@JeffBezanson
Copy link
Member Author

This now includes #23240, but using front-end linearization instead of its own linearize pass. Seems to help clean up some of the extra allocations in the benchmarks here. Let's see.

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@JeffBezanson JeffBezanson force-pushed the jb/linear-ir-1 branch 2 times, most recently from b6a8088 to ec20c0b Compare November 16, 2017 22:39
@JeffBezanson
Copy link
Member Author

Ok, it appears that adding some @yuyichao magic has indeed fixed the remaining performance regressions. I worked through a couple more bugs and I think this is working now.

@StefanKarpinski
Copy link
Member

Everything’s coming up Milhouse!

@JeffBezanson
Copy link
Member Author

This recent test is failing on this branch:

@test contains(get_llvm_noopt(foo24632, (Bool,), false), "!dereferenceable_or_null")

The reason appears to be that the better optimizations here are able to eliminate the x variable. This:

        x = #temp#@_5
        #= line 319 =#
        unless (x isa Main.Int)::Bool goto 25

became this:

        SSAValue(2) = #temp#@_5
        #= line 319 =#
        SSAValue(4) = (SSAValue(2) isa Main.Int)::Bool
        unless SSAValue(4) goto 27

I'm not sure why x gets this annotation but the temp variable it's initialized from doesn't (in the first version of the code).

@JeffBezanson
Copy link
Member Author

Ok, I believe I've fixed that, by avoiding replacing TypedSlots. @yuyichao would be good if you can take a look at this.

@JeffBezanson
Copy link
Member Author

... In particular, in my latest commit I wasn't fully sure whether to return true or false.

@JeffBezanson JeffBezanson force-pushed the jb/linear-ir-1 branch 2 times, most recently from 292f959 to 3a9ed80 Compare December 2, 2017 22:31
@quinnj
Copy link
Member

quinnj commented Dec 6, 2017

Looks like this needs a rebase; but the last CI run had quite a bit of green.

JeffBezanson and others added 4 commits December 8, 2017 23:55
These objects make it really hard to mutate the AST correctly
since one mutation can be accidentally done at places where it is invalid.
The hardest part for running non-local optimization passes
(i.e. the transformation does not rely only on one or a few neighboring expressions)
is to avoid re-analyse the code. Our current IR, though easy for linear scanning,
interpreting, codegen and, to a certain degree, storage, is not very easy for making random
updates. Try to workaround this issue in two ways,

1. Never resize the code array when doing updates.
   Instead, inserting nested arrays that we'll later splice back in for code addition and
   use `nothing` for code deletion. This way, the array index we cached for other metadata
   about the code can stay valid.
2. Based on the previous approach, pre-scan the use-def info for all variables before starting
   the optimization and run the optimization recursively.
   Code changes will also update this use-def data so that it's always valid for the user.
   Changes that can affect the use or def of another value will re-trigger the optimization
   so that we can take advantage of new optimization opportunities.

This optimization pass should now handle most of the control-flow insensitive optimizations.
Code patterns that are handled partially by this pass but will benefit greatly from an
control-flow sensitive version includes,

1. Split slots (based on control flow)

   This way we can completely eliminate the surprising cost due to variable name conflicts,
   even when one of the def-use is not type stable.
   (This pass currently handles the case where all the def/uses are type stable)

2. Delay allocations

   There are cases where the allocation escapes but only in some branches.
   This will be especially for error path since we cannot eliminate some `SubArray` allocation
   only because we want to maintain them for the bounds error. This is very stupid and we should
   be able to do the allocation only when we throw the error, leaving the performance critical
   non-error path allocation-free.

3. Reordering assignments

   It is in general illegal to move an assignment when the slot assigned to is not SSA.
   However, there are many case that is actually legal
   (i.e. if there's no other use or def in between) to do so. This shows up a lot in code like

   ```
   SSA = alloc
   slot = SSA
   ```

   which we currently can't optimize since the slot can't see the assignment is actually
   an allocation and not a generic black box. We should be able to merge this and eliminate the
   SSA based on control flow info. For this case, a def info that looks through SSA values
   can also help.
@Keno Keno merged commit 66b2090 into master Dec 11, 2017
@StefanKarpinski StefanKarpinski deleted the jb/linear-ir-1 branch December 11, 2017 19:52
@Sacha0
Copy link
Member

Sacha0 commented Dec 11, 2017

🎉!

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.

9 participants