-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
correct codegen bugs introduced by allocation-hoisting-PR #45476
Changes from all commits
4a99a31
a3cb97b
508f292
ce8ff75
30f68bc
e366acb
76671d1
ac65255
d645429
4cac1d0
7558018
e4257cb
2e8363b
9fcb436
088b36c
f8f7028
d653492
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1005,7 +1005,7 @@ static LoadInst *emit_nthptr_recast(jl_codectx_t &ctx, Value *v, ssize_t n, MDNo | |
emit_bitcast(ctx, vptr, PointerType::get(type, 0))))); | ||
} | ||
|
||
static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &v); | ||
static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &v, bool is_promotable=false); | ||
static Value *emit_typeof(jl_codectx_t &ctx, Value *v, bool maybenull); | ||
|
||
static jl_cgval_t emit_typeof(jl_codectx_t &ctx, const jl_cgval_t &p, bool maybenull) | ||
|
@@ -3207,6 +3207,44 @@ static Value *box_union(jl_codectx_t &ctx, const jl_cgval_t &vinfo, const SmallB | |
return box_merge; | ||
} | ||
|
||
static Function *mangleIntrinsic(IntrinsicInst *call) //mangling based on replaceIntrinsicUseWith | ||
{ | ||
Intrinsic::ID ID = call->getIntrinsicID(); | ||
auto nargs = call->arg_size(); | ||
SmallVector<Type*, 8> argTys(nargs); | ||
auto oldfType = call->getFunctionType(); | ||
for (unsigned i = 0; i < oldfType->getNumParams(); i++) { | ||
auto argi = call->getArgOperand(i); | ||
argTys[i] = argi->getType(); | ||
} | ||
|
||
auto newfType = FunctionType::get( | ||
oldfType->getReturnType(), | ||
makeArrayRef(argTys).slice(0, oldfType->getNumParams()), | ||
oldfType->isVarArg()); | ||
|
||
// Accumulate an array of overloaded types for the given intrinsic | ||
// and compute the new name mangling schema | ||
SmallVector<Type*, 4> overloadTys; | ||
{ | ||
SmallVector<Intrinsic::IITDescriptor, 8> Table; | ||
getIntrinsicInfoTableEntries(ID, Table); | ||
ArrayRef<Intrinsic::IITDescriptor> TableRef = Table; | ||
auto res = Intrinsic::matchIntrinsicSignature(newfType, TableRef, overloadTys); | ||
assert(res == Intrinsic::MatchIntrinsicTypes_Match); | ||
(void)res; | ||
bool matchvararg = !Intrinsic::matchIntrinsicVarArg(newfType->isVarArg(), TableRef); | ||
assert(matchvararg); | ||
(void)matchvararg; | ||
} | ||
auto newF = Intrinsic::getDeclaration(call->getModule(), ID, overloadTys); | ||
assert(newF->getFunctionType() == newfType); | ||
newF->setCallingConv(call->getCallingConv()); | ||
return newF; | ||
} | ||
|
||
|
||
//Used for allocation hoisting in *boxed | ||
static void recursively_adjust_ptr_type(llvm::Value *Val, unsigned FromAS, unsigned ToAS) | ||
{ | ||
for (auto *User : Val->users()) { | ||
|
@@ -3216,13 +3254,8 @@ static void recursively_adjust_ptr_type(llvm::Value *Val, unsigned FromAS, unsig | |
recursively_adjust_ptr_type(Inst, FromAS, ToAS); | ||
} | ||
else if (isa<IntrinsicInst>(User)) { | ||
IntrinsicInst *II = cast<IntrinsicInst>(User); | ||
SmallVector<Type*, 3> ArgTys; | ||
Intrinsic::getIntrinsicSignature(II->getCalledFunction(), ArgTys); | ||
assert(ArgTys.size() <= II->arg_size()); | ||
for (size_t i = 0; i < ArgTys.size(); ++i) | ||
ArgTys[i] = II->getArgOperand(i)->getType(); | ||
II->setCalledFunction(Intrinsic::getDeclaration(II->getModule(), II->getIntrinsicID(), ArgTys)); | ||
IntrinsicInst *call = cast<IntrinsicInst>(User); | ||
call->setCalledFunction(mangleIntrinsic(call)); | ||
} | ||
#ifndef JL_LLVM_OPAQUE_POINTERS | ||
else if (isa<BitCastInst>(User)) { | ||
|
@@ -3238,7 +3271,7 @@ static void recursively_adjust_ptr_type(llvm::Value *Val, unsigned FromAS, unsig | |
// dynamically-typed value is required (e.g. argument to unknown function). | ||
// if it's already a pointer it's left alone. | ||
// Returns ctx.types().T_prjlvalue | ||
static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &vinfo) | ||
static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &vinfo, bool is_promotable) | ||
{ | ||
jl_value_t *jt = vinfo.typ; | ||
if (jt == jl_bottom_type || jt == NULL) | ||
|
@@ -3268,7 +3301,7 @@ static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &vinfo) | |
box = _boxed_special(ctx, vinfo, t); | ||
if (!box) { | ||
bool do_promote = vinfo.promotion_point; | ||
if (do_promote) { | ||
if (do_promote && is_promotable) { | ||
auto IP = ctx.builder.saveIP(); | ||
ctx.builder.SetInsertPoint(vinfo.promotion_point); | ||
box = emit_allocobj(ctx, jl_datatype_size(jt), literal_pointer_val(ctx, (jl_value_t*)jt)); | ||
|
@@ -3282,7 +3315,7 @@ static Value *boxed(jl_codectx_t &ctx, const jl_cgval_t &vinfo) | |
recursively_adjust_ptr_type(originalAlloca, 0, AddressSpace::Derived); | ||
originalAlloca->replaceAllUsesWith(decayed); | ||
// end illegal IR | ||
cast<Instruction>(vinfo.V)->eraseFromParent(); | ||
originalAlloca->eraseFromParent(); | ||
ctx.builder.restoreIP(IP); | ||
} else { | ||
box = emit_allocobj(ctx, jl_datatype_size(jt), literal_pointer_val(ctx, (jl_value_t*)jt)); | ||
|
@@ -3651,7 +3684,7 @@ static jl_cgval_t emit_new_struct(jl_codectx_t &ctx, jl_value_t *ty, size_t narg | |
if (fval_info.typ == jl_bottom_type) | ||
return jl_cgval_t(); | ||
// TODO: Use (post-)domination instead. | ||
bool field_promotable = !init_as_value && fval_info.promotion_ssa != -1 && | ||
bool field_promotable = !jl_is_uniontype(jtype) && !init_as_value && fval_info.promotion_ssa != -1 && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be valid though if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue is that unions currently don't get their promotion point correctly, I'm not exactly what's needed to fix that, so for now, not allowing unions at all seems fine. |
||
fval_info.promotion_point && fval_info.promotion_point->getParent() == ctx.builder.GetInsertBlock(); | ||
if (field_promotable) { | ||
savedIP = ctx.builder.saveIP(); | ||
|
@@ -3684,20 +3717,20 @@ static jl_cgval_t emit_new_struct(jl_codectx_t &ctx, jl_value_t *ty, size_t narg | |
promotion_point = inst; | ||
promotion_ssa = fval_info.promotion_ssa; | ||
} | ||
} else if (!promotion_point) { | ||
} | ||
else if (!promotion_point) { | ||
promotion_point = inst; | ||
} | ||
} | ||
Value *fval = NULL; | ||
if (jl_field_isptr(sty, i)) { | ||
fval = boxed(ctx, fval_info); | ||
fval = boxed(ctx, fval_info, field_promotable); | ||
if (!init_as_value) | ||
cast<StoreInst>(tbaa_decorate(ctx.tbaa().tbaa_stack, | ||
ctx.builder.CreateAlignedStore(fval, dest, Align(jl_field_align(sty, i))))) | ||
->setOrdering(AtomicOrdering::Unordered); | ||
} | ||
else if (jl_is_uniontype(jtype)) { | ||
assert(!field_promotable); | ||
// compute tindex from rhs | ||
jl_cgval_t rhs_union = convert_julia_type(ctx, fval_info, jtype); | ||
if (rhs_union.typ == jl_bottom_type) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So promotion is now always forbidden implicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the regression we were seeing was from something getting promoted when we called
boxed
on a separate part fo the of the compilation and then hoist something that shouldn't be hoisted.In the case of the regression we were hosting a string allocation from inside an error to the main path of the function which then impeded an optimization.
From what I understand we should only hoist the allocation when we have nested structs and it's fields are promotable which we only check inside
emit_new_struct