Skip to content

Commit

Permalink
optimizer: fix alloc opt on unknown offset with references (#47076)
Browse files Browse the repository at this point in the history
Fixes issued mentioned in #47075 (comment)
  • Loading branch information
wsmoses authored Oct 10, 2022
1 parent 1cb70ff commit a68235c
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 5 deletions.
14 changes: 12 additions & 2 deletions src/llvm-alloc-helpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,12 @@ void jl_alloc::runEscapeAnalysis(llvm::Instruction *I, EscapeAnalysisRequiredArg
auto check_inst = [&] (Instruction *inst, Use *use) {
if (isa<LoadInst>(inst)) {
required.use_info.hasload = true;
if (cur.offset == UINT32_MAX || !required.use_info.addMemOp(inst, 0, cur.offset,
if (cur.offset == UINT32_MAX) {
auto elty = inst->getType();
required.use_info.has_unknown_objref |= hasObjref(elty);
required.use_info.has_unknown_objrefaggr |= hasObjref(elty) && !isa<PointerType>(elty);
required.use_info.hasunknownmem = true;
} else if (!required.use_info.addMemOp(inst, 0, cur.offset,
inst->getType(),
false, required.DL))
required.use_info.hasunknownmem = true;
Expand Down Expand Up @@ -232,7 +237,12 @@ void jl_alloc::runEscapeAnalysis(llvm::Instruction *I, EscapeAnalysisRequiredArg
return false;
}
auto storev = store->getValueOperand();
if (cur.offset == UINT32_MAX || !required.use_info.addMemOp(inst, use->getOperandNo(),
if (cur.offset == UINT32_MAX) {
auto elty = storev->getType();
required.use_info.has_unknown_objref |= hasObjref(elty);
required.use_info.has_unknown_objrefaggr |= hasObjref(elty) && !isa<PointerType>(elty);
required.use_info.hasunknownmem = true;
} else if (!required.use_info.addMemOp(inst, use->getOperandNo(),
cur.offset, storev->getType(),
true, required.DL))
required.use_info.hasunknownmem = true;
Expand Down
7 changes: 7 additions & 0 deletions src/llvm-alloc-helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ namespace jl_alloc {
// The object is used in an error function
bool haserror:1;

// The alloc has a Julia object reference not in an explicit field.
bool has_unknown_objref:1;
// The alloc has an aggregate Julia object reference not in an explicit field.
bool has_unknown_objrefaggr:1;

void reset()
{
escaped = false;
Expand All @@ -99,6 +104,8 @@ namespace jl_alloc {
hasunknownmem = false;
returned = false;
haserror = false;
has_unknown_objref = false;
has_unknown_objrefaggr = false;
uses.clear();
preserves.clear();
memops.clear();
Expand Down
8 changes: 5 additions & 3 deletions src/llvm-alloc-opt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,8 @@ void Optimizer::optimizeAll()
removeAlloc(orig);
continue;
}
bool has_ref = false;
bool has_refaggr = false;
bool has_ref = use_info.has_unknown_objref;
bool has_refaggr = use_info.has_unknown_objrefaggr;
for (auto memop: use_info.memops) {
auto &field = memop.second;
if (field.hasobjref) {
Expand Down Expand Up @@ -576,7 +576,9 @@ void Optimizer::moveToStack(CallInst *orig_inst, size_t sz, bool has_ref)
// treat this as a non-mem2reg'd alloca
// The ccall root and GC preserve handling below makes sure that
// the alloca isn't optimized out.
buff = prolog_builder.CreateAlloca(pass.T_prjlvalue);
const DataLayout &DL = F.getParent()->getDataLayout();
auto asize = ConstantInt::get(Type::getInt64Ty(prolog_builder.getContext()), sz / DL.getTypeAllocSize(pass.T_prjlvalue));
buff = prolog_builder.CreateAlloca(pass.T_prjlvalue, asize);
buff->setAlignment(Align(align));
ptr = cast<Instruction>(prolog_builder.CreateBitCast(buff, Type::getInt8PtrTy(prolog_builder.getContext())));
}
Expand Down
35 changes: 35 additions & 0 deletions test/llvmpasses/alloc-opt-unsized.ll
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
; RUN: opt -enable-new-pm=0 -load libjulia-codegen%shlibext -AllocOpt -S %s | FileCheck %s

source_filename = "text"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128-ni:10:11:12:13"
target triple = "x86_64-linux-gnu"

declare {}*** @julia.get_pgcstack()

declare {} addrspace(10)* @julia.gc_alloc_obj({}**, i64, {} addrspace(10)*)

declare void @julia.write_barrier({} addrspace(10)*, ...)

define void @diffejulia_objective__1864_inner_1wrap({} addrspace(10)* %arg, i64 %iv.i) {
entry:
%i5 = call {}*** @julia.get_pgcstack()
%i13 = bitcast {}*** %i5 to {}**
%i14 = getelementptr inbounds {}*, {}** %i13, i64 -12
%i18 = call noalias nonnull dereferenceable(8000) dereferenceable_or_null(8000) {} addrspace(10)* @julia.gc_alloc_obj({}** %i14, i64 8000, {} addrspace(10)* addrspacecast ({}* inttoptr (i64 139756155247504 to {}*) to {} addrspace(10)*))
%_malloccache.i = bitcast {} addrspace(10)* %i18 to {} addrspace(10)* addrspace(10)*
%i23 = getelementptr inbounds {} addrspace(10)*, {} addrspace(10)* addrspace(10)* %_malloccache.i, i64 %iv.i
store {} addrspace(10)* %arg, {} addrspace(10)* addrspace(10)* %i23, align 8
%i24 = bitcast {} addrspace(10)* addrspace(10)* %_malloccache.i to {} addrspace(10)*
call void ({} addrspace(10)*, ...) @julia.write_barrier({} addrspace(10)* %i24, {} addrspace(10)* %arg)
%l = load {} addrspace(10)*, {} addrspace(10)* addrspace(10)* %i23
ret void
}

; CHECK: %[[i0:.+]] = alloca {} addrspace(10)*, i64 1000, align 16
; CHECK: %[[i1:.+]] = bitcast {} addrspace(10)** %[[i0]] to i8*
; CHECK: %i18 = bitcast i8* %[[i1]] to {}*
; CHECK: %_malloccache.i = bitcast {}* %i18 to {} addrspace(10)**
; CHECK: %i23 = getelementptr inbounds {} addrspace(10)*, {} addrspace(10)** %_malloccache.i, i64 %iv.i
; CHECK: store {} addrspace(10)* %arg, {} addrspace(10)** %i23, align 8
; CHECK: %i24 = bitcast {} addrspace(10)** %_malloccache.i to {}*
; CHECK: %l = load {} addrspace(10)*, {} addrspace(10)** %i23, align 8

4 comments on commit a68235c

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true, configuration=(rr=true,))

@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 package evaluation job has completed - possible new issues were detected. A full report can be found here.

@vtjnash
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@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.

Please sign in to comment.