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.

Sign up for GitHub

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

Jump to bottom

Avoid boxing variables that may be used undef #18732

Merged
merged 1 commit into from
Oct 12, 2016
Merged

Avoid boxing variables that may be used undef #18732

merged 1 commit into from
Oct 12, 2016

Conversation

carnaval
Copy link
Contributor

use a separate i1 flag so that we avoid boxing and give visibility to llvm in the defined/undefined dataflow, to remove redundant branches etc

f(a,b) = (if a; c = b; end; c)

gives

define i64 @julia_f_65179(i8, i64) #0 {
top:
  %2 = and i8 %0, 1
  %3 = icmp eq i8 %2, 0
  br i1 %3, label %err, label %ok

err:                                              ; preds = %top
  call void @jl_undefined_var_error(%jl_value_t* inttoptr (i64 140728621134648 to %jl_value_t*))
  unreachable

ok:                                               ; preds = %top
  ret i64 %1
}

JeffBezanson
JeffBezanson approved these changes Sep 29, 2016
View reviewed changes
src/codegen.cpp
@@ -490,6 +490,9 @@ struct jl_varinfo_t {
#else
DIVariable dinfo;
#endif
// if the variable might be used undefined and is not boxed
// this i1 flag is true when it is defined
Value *undefFlag;
Copy link
Member

Choose a reason for hiding this comment

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

I suggest calling it defFlag to avoid double negatives :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that was gratuitously confusing

@JeffBezanson
Copy link
Member

Awesome; this has been a long time coming.

yuyichao
yuyichao reviewed Sep 29, 2016
View reviewed changes
src/codegen.cpp
static void alloc_undef_flag(jl_varinfo_t& vi, jl_codectx_t* ctx)
{
if (vi.usedUndef) {
vi.undefFlag = emit_static_alloca(T_int1, ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need this if the slot is a isbits type right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or a constant. It should only be called in those cases. I'll add an assertion.

@tkelman
Copy link
Contributor

tkelman commented Sep 30, 2016

we need to remember to do this more often: @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 @jrevels

StefanKarpinski
StefanKarpinski requested changes Sep 30, 2016
View reviewed changes
src/codegen.cpp
@@ -737,6 +741,22 @@ static jl_sym_t *slot_symbol(int s, jl_codectx_t *ctx)
return (jl_sym_t*)jl_array_ptr_ref(ctx->source->slotnames, s);
}

static void store_undef_flag(const jl_varinfo_t &vi, bool val)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these accessors be called store_def_flag now?

src/codegen.cpp
builder.CreateStore(ConstantInt::get(T_int1, val), vi.defFlag);
}

static void alloc_undef_flag(jl_varinfo_t& vi, jl_codectx_t* ctx)
Copy link
Member

Choose a reason for hiding this comment

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

Same: alloc_undef_flag => alloc_def_flag

@JeffBezanson
Copy link
Member

Bump. Just needs a bit more renaming, then good to go.

@JeffBezanson
Copy link
Member

Ok, did it myself :P

@JeffBezanson
Copy link
Member

Fixes #6914

@JeffBezanson JeffBezanson merged commit 11d3c96 into master Oct 12, 2016
@JeffBezanson JeffBezanson mentioned this pull request Oct 12, 2016
Closed
@tkelman tkelman deleted the ob/udvopt branch October 12, 2016 06:35
@vtjnash
Copy link
Member

vtjnash commented Oct 17, 2016

I think there was a bug in this.

Assertion failed: (vi.defFlag), function emit_local, file /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp, line 3142.

signal (4): Illegal instruction: 4
while loading /Volumes/Lion/Users/jameson/Documents/julia/base/precompile.jl, in expression starting on line 480
abort at /Volumes/Lion/Users/jameson/Documents/julia/build-llvm37/usr/lib//libLLVM-3.7.dylib (unknown line)
__assert_rtn at /Volumes/Lion/Users/jameson/Documents/julia/build-llvm37/usr/lib//libLLVM-3.7.dylib (unknown line)
emit_local at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:3142 [inlined]
emit_expr at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:3338
emit_assignment at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:3217 [inlined]
emit_expr at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:3421
emit_stmtpos at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:3327 [inlined]
emit_function at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:5081
jl_compile_linfo at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:927
jl_generate_fptr at /Volumes/Lion/Users/jameson/Documents/julia/src/codegen.cpp:1170

CodeInfo(code=Array{Any, 1}[
  # line 312,
  Expr(:meta, :push_loc, :multidimensional.jl)::Any,
  # line 315,
  SlotNumber(id=10) = SlotNumber(id=3),
  SlotNumber(id=8) = SlotNumber(id=1),
  SlotNumber(id=9) = SlotNumber(id=2),
  SlotNumber(id=11) = SlotNumber(id=4),
  SlotNumber(id=5) = Expr(:call, Base.to_index, Expr(:call, Base.getindex, SlotNumber(id=11), 1)::Any)::Any,
  # line 316,
  SlotNumber(id=6) = Expr(:call, Base.index_shape, SlotNumber(id=10), SlotNumber(id=5))::Any,
  # line 317,
  SlotNumber(id=7) = Expr(:call, Base.similar, SlotNumber(id=10), SlotNumber(id=6))::Any,
  # line 318,
  SSAValue(0) = Expr(:call, Base.==, Expr(:call, Base.map, Base.unsafe_length, Expr(:call, Base.indices, SlotNumber(id=7))::Any)::Any, Expr(:call, Base.map, Base.unsafe_length, SlotNumber(id=6))::Any)::Any,
  Expr(:gotoifnot, SSAValue(0), 17)::Any,
  goto 19,
  17:,
  Expr(:call, Base.throw_checksize_error, SlotNumber(id=7), SlotNumber(id=6))::Any,
  19:,
  # line 319,
  SSAValue(1) = Expr(:call, Base._unsafe_getindex!, SlotNumber(id=7), SlotNumber(id=10), SlotNumber(id=5))::Any,
  Expr(:meta, :pop_loc)::Any,
  Expr(:return, SSAValue(1))::Any], slottypes=nothing, ssavaluetypes=2, slotnames=Array{Any, 1}[
  :#self#,
  :#unused#,
  :A,
  :I,
  :I_1,
  :shape,
  :dest,
  :#self#,
  :#unused#,
  :A,
  :I], slotflags=Array{UInt8, 1}[0x02, 0x02, 0x02, 0x02, 0x12, 0x12, 0x12, 0x02, 0x02, 0x02, 0x02], inferred=false, inlineable=false, propagate_inbounds=false, pure=false)

@JeffBezanson
Copy link
Member

I have a fix.

@JeffBezanson
Copy link
Member

#19036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuyichao yuyichao yuyichao left review comments

@StefanKarpinski StefanKarpinski StefanKarpinski requested changes

@JeffBezanson JeffBezanson JeffBezanson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@carnaval @JeffBezanson @tkelman @nanosoldier @vtjnash @StefanKarpinski @yuyichao