Skip to content

Commit

Permalink
llvm-alloc-opt: handle dead code better (#39801)
Browse files Browse the repository at this point in the history
In some cases (particularly after removing a phi node), we might end up
in a circumstance where it appears statically that we would use a slot
for both a ref and bits. Avoid generating malformed IR in this case.

In the future, this situation could also possibly happen if we walked
through phi nodes and attempted to merge the contents (with great care).
  • Loading branch information
vtjnash authored Mar 2, 2021
1 parent 4f36351 commit 9e783bb
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 12 deletions.
23 changes: 11 additions & 12 deletions src/llvm-alloc-opt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,8 @@ void Optimizer::optimizeAll()
if (field.hasobjref) {
has_ref = true;
// This can be relaxed a little based on hasload
if (field.hasaggr || field.multiloc) {
// TODO: add support for hasaggr load/store
if (field.hasaggr || field.multiloc || field.size != sizeof(void*)) {
has_refaggr = true;
break;
}
Expand All @@ -361,15 +362,12 @@ void Optimizer::optimizeAll()
splitOnStack(orig);
continue;
}
if (has_ref) {
if (use_info.memops.size() != 1 || has_refaggr ||
use_info.memops.begin()->second.size != sz) {
if (use_info.hastypeof)
optimizeTag(orig);
continue;
}
// The object only has a single field that's a reference with only one kind of access.
if (has_refaggr) {
if (use_info.hastypeof)
optimizeTag(orig);
continue;
}
// The object has no fields with mix reference access
moveToStack(orig, sz, has_ref);
}
}
Expand Down Expand Up @@ -444,6 +442,7 @@ Optimizer::AllocUseInfo::getField(uint32_t offset, uint32_t size, Type *elty)
if (it->first + it->second.size >= offset + size) {
if (it->second.elty != elty)
it->second.elty = nullptr;
assert(it->second.elty == nullptr || (it->first == offset && it->second.size == size));
return *it;
}
if (it->first + it->second.size > offset) {
Expand All @@ -454,7 +453,7 @@ Optimizer::AllocUseInfo::getField(uint32_t offset, uint32_t size, Type *elty)
else {
it = memops.begin();
}
// Now fine the last slot that overlaps with the current memory location.
// Now find the last slot that overlaps with the current memory location.
// Also set `lb` if we didn't find any above.
for (; it != end && it->first < offset + size; ++it) {
if (lb == end)
Expand Down Expand Up @@ -493,8 +492,8 @@ bool Optimizer::AllocUseInfo::addMemOp(Instruction *inst, unsigned opno, uint32_
memop.isaggr = isa<StructType>(elty) || isa<ArrayType>(elty) || isa<VectorType>(elty);
memop.isobjref = hasObjref(elty);
auto &field = getField(offset, size, elty);
if (field.first != offset || field.second.size != size)
field.second.multiloc = true;
if (field.second.hasobjref != memop.isobjref)
field.second.multiloc = true; // can't split this field, since it contains a mix of references and bits
if (!isstore)
field.second.hasload = true;
if (memop.isobjref) {
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,34 @@ declare void @llvm.memcpy.p11i8.p0i8.i64(i8 addrspace(11)* nocapture writeonly,
declare token @llvm.julia.gc_preserve_begin(...)
declare void @llvm.julia.gc_preserve_end(token)
""")

# CHECK-LABEL: @memref_collision
# CHECK: call {}*** @julia.ptls_states()
# CHECK-NOT: store {}
# CHECK: store i
# CHECK-NOT: store {}
# CHECK: L1:
# CHECK: load {}
# CHECK: L2:
# CHECK: load i
println("""
define void @memref_collision($isz %x) {
%ptls = call {}*** @julia.ptls_states()
%ptls_i8 = bitcast {}*** %ptls to i8*
%v = call noalias {} addrspace(10)* @julia.gc_alloc_obj(i8* %ptls_i8, $isz 8, {} addrspace(10)* @tag)
%v_p = bitcast {} addrspace(10)* %v to $isz addrspace(10)*
store $isz %x, $isz addrspace(10)* %v_p
br i1 0, label %L1, label %L2
L1:
%v1 = bitcast {} addrspace(10)* %v to {} addrspace(10)* addrspace(10)*
%v1_x = load {} addrspace(10)*, {} addrspace(10)* addrspace(10)* %v1
ret void
L2:
%v2 = bitcast {} addrspace(10)* %v to $isz addrspace(10)*
%v2_x = load i64, i64 addrspace(10)* %v2
ret void
}
""")
# CHECK-LABEL: }{{$}}
File renamed without changes.

7 comments on commit 9e783bb

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks("string", vs="@3ff44eab64c554b31a87df1b972e99959ad15e54")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: TaskFailedException:
failed process: Process(setenv(`make -j7 --output-sync=target JULIA_PRECOMPILE=0`; dir="/run/media/system/data/nanosoldier/workdir/jl_ORNYXF"), ProcessExited(2)) [2]

pipeline_error at ./process.jl:525 [inlined]
#run#596 at ./process.jl:440
run at ./process.jl:438 [inlined]
build_julia! at /run/media/system/data/nanosoldier/Nanosoldier.jl/src/build.jl:93
build_julia! at /run/media/system/data/nanosoldier/Nanosoldier.jl/src/build.jl:47 [inlined]
build_benchmarksjulia! at /run/media/system/data/nanosoldier/Nanosoldier.jl/src/jobs/BenchmarkJob.jl:301
#17 at ./task.jl:356

Logs and partial data can be found here
cc @christopher-dG

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks("string", vs="@4f36351eb95240b87bc6ab41de65a90bf7726f18")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: TaskFailedException:
failed process: Process(setenv(`make -j7 --output-sync=target JULIA_PRECOMPILE=0`; dir="/run/media/system/data/nanosoldier/workdir/jl_8MaU0E"), ProcessExited(2)) [2]

pipeline_error at ./process.jl:525 [inlined]
#run#596 at ./process.jl:440
run at ./process.jl:438 [inlined]
build_julia! at /run/media/system/data/nanosoldier/Nanosoldier.jl/src/build.jl:93
build_julia! at /run/media/system/data/nanosoldier/Nanosoldier.jl/src/build.jl:47 [inlined]
build_benchmarksjulia! at /run/media/system/data/nanosoldier/Nanosoldier.jl/src/jobs/BenchmarkJob.jl:301
#17 at ./task.jl:356

Logs and partial data can be found here
cc @christopher-dG

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks("string", vs="@4f36351eb95240b87bc6ab41de65a90bf7726f18")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, apparently this was the commit that impacted that benchmark. Since that was secondarily the goal of this PR, I think we're okay.

Please sign in to comment.