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

Fix Proc#call that takes and returns large extern structs by value #14323

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions spec/compiler/codegen/extern_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,29 @@ describe "Codegen: extern struct" do
)).to_i.should eq(3)
end

it "codegens proc that takes and returns large extern struct by value" do
run(<<-CRYSTAL).to_i.should eq(149)
@[Extern]
struct Foo
@unused = uninitialized Int64

def initialize(@x : Int32, @y : Int32, @z : Int32)
end
end

f = ->(foo : Foo) {
Foo.new(foo.@x, foo.@y &* 2, foo.@z &* 3)
}

foo = f.call(Foo.new(100, 20, 3))
foo.@x &+ foo.@y &+ foo.@z
CRYSTAL
end

# These specs *should* also work for 32 bits, but for now we'll
# make sure they work in 64 bits (they probably work in 32 bits too,
# it's just that the specs need to be a bit different)
{% if flag?(:x86_64) %}
{% if flag?(:x86_64) || flag?(:aarch64) %}
HertzDevil marked this conversation as resolved.
Show resolved Hide resolved
it "codegens proc that takes an extern struct with C ABI" do
test_c(
%(
Expand Down Expand Up @@ -427,7 +446,7 @@ describe "Codegen: extern struct" do
), &.to_i.should eq(30))
end

pending_win32 "codegens proc that takes and returns an extern struct with sret" do
it "codegens proc that takes and returns an extern struct with sret" do
test_c(
%(
struct Struct {
Expand Down
2 changes: 1 addition & 1 deletion spec/std/llvm/aarch64_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ class LLVM::ABI
info.return_type.should eq(ArgType.direct(str, cast: ctx.int64.array(2)))
end

test "does with structs between 64 and 128 bits" do |abi, ctx|
test "does with structs larger than 128 bits" do |abi, ctx|
str = ctx.struct([ctx.int64, ctx.int64, ctx.int8])
arg_types = [str]
return_type = str
Expand Down
44 changes: 37 additions & 7 deletions spec/std/llvm/x86_64_abi_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,17 @@ require "llvm"
LLVM.init_x86
{% end %}

private def abi
triple = LLVM.default_target_triple.gsub(/^(.+?)-/, "x86_64-")
private def abi(win64 = false)
triple = win64 ? "x86_64-windows-msvc" : LLVM.default_target_triple.gsub(/^(.+?)-/, "x86_64-")
target = LLVM::Target.from_triple(triple)
machine = target.create_target_machine(triple)
machine.enable_global_isel = false
LLVM::ABI::X86_64.new(machine)
win64 ? LLVM::ABI::X86_Win64.new(machine) : LLVM::ABI::X86_64.new(machine)
end

private def test(msg, &block : LLVM::ABI, LLVM::Context ->)
it msg do
abi = abi()
private def test(msg, *, win64 = false, file = __FILE__, line = __LINE__, &block : LLVM::ABI, LLVM::Context ->)
it msg, file: file, line: line do
abi = abi(win64)
ctx = LLVM::Context.new
block.call(abi, ctx)
end
Expand Down Expand Up @@ -133,7 +133,37 @@ class LLVM::ABI
info.return_type.should eq(ArgType.direct(str, cast: ctx.struct([ctx.int64, ctx.int64])))
end

test "does with structs between 64 and 128 bits" do |abi, ctx|
test "does with structs larger than 128 bits" do |abi, ctx|
str = ctx.struct([ctx.int64, ctx.int64, ctx.int8])
arg_types = [str]
return_type = str

info = abi.abi_info(arg_types, return_type, true, ctx)
info.arg_types.size.should eq(1)

info.arg_types[0].should eq(ArgType.indirect(str, Attribute::ByVal))
info.return_type.should eq(ArgType.indirect(str, Attribute::StructRet))
end
end
{% end %}
end

describe X86_Win64 do
{% if LibLLVM::BUILT_TARGETS.includes?(:x86) %}
describe "abi_info" do
test "does with structs between 64 and 128 bits", win64: true do |abi, ctx|
str = ctx.struct([ctx.int64, ctx.int16])
arg_types = [str]
return_type = str

info = abi.abi_info(arg_types, return_type, true, ctx)
info.arg_types.size.should eq(1)

info.arg_types[0].should eq(ArgType.indirect(str, Attribute::ByVal))
info.return_type.should eq(ArgType.indirect(str, Attribute::StructRet))
end

test "does with structs larger than 128 bits", win64: true do |abi, ctx|
str = ctx.struct([ctx.int64, ctx.int64, ctx.int8])
arg_types = [str]
return_type = str
Expand Down
7 changes: 7 additions & 0 deletions src/compiler/crystal/codegen/call.cr
Original file line number Diff line number Diff line change
Expand Up @@ -597,13 +597,20 @@ class Crystal::CodeGenVisitor
abi_info = abi_info(target_def)
end

sret = abi_info && sret?(abi_info)
arg_offset = is_closure ? 2 : 1
arg_offset += 1 if sret

arg_types = fun_type.try(&.arg_types) || target_def.try &.args.map &.type
arg_types.try &.each_with_index do |arg_type, i|
if abi_info && (abi_arg_type = abi_info.arg_types[i]?) && (attr = abi_arg_type.attr)
@last.add_instruction_attribute(i + arg_offset, attr, llvm_context, abi_arg_type.type)
end
end

if abi_info && sret
@last.add_instruction_attribute(1, LLVM::Attribute::StructRet, llvm_context, abi_info.return_type.type)
end
end

def sret?(abi_info)
Expand Down
5 changes: 4 additions & 1 deletion src/compiler/crystal/codegen/fun.cr
Original file line number Diff line number Diff line change
Expand Up @@ -535,11 +535,14 @@ class Crystal::CodeGenVisitor
value = pointer2
end
else
abi_info = target_def.abi_info? ? abi_info(target_def) : nil
sret = abi_info && sret?(abi_info)

# If it's an extern struct on a def that must be codegened with C ABI
# compatibility, and it's not passed byval, we must cast the value
if target_def.c_calling_convention? && arg.type.extern? && !context.fun.attributes(index + 1).by_val?
# ... unless it's passed indirectly (ie. as a pointer to memory allocated by the caller)
if target_def.abi_info? && abi_info(target_def).arg_types[index].kind.indirect?
if abi_info.try &.arg_types[index - (sret ? 1 : 0)].kind.indirect?
value = declare_debug_for_function_argument(arg.name, var_type, index + 1, value, location) unless target_def.naked?
context.vars[arg.name] = LLVMVar.new(value, var_type)
else
Expand Down
2 changes: 1 addition & 1 deletion src/llvm/abi/x86_win64.cr
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class LLVM::ABI::X86_Win64 < LLVM::ABI::X86
when 2 then ArgType.direct(t, context.int16)
when 4 then ArgType.direct(t, context.int32)
when 8 then ArgType.direct(t, context.int64)
else ArgType.indirect(t, nil)
else ArgType.indirect(t, LLVM::Attribute::ByVal)
end
else
non_struct(t, context)
Expand Down
Loading