Compiler: don't use byval in codegen

crystal-lang · Oct 28, 2016 · 6f6ddc0 · 6f6ddc0
1 parent 36b2fc4
commit 6f6ddc0
Showing 7 changed files with 120 additions and 111 deletions.
diff --git a/src/compiler/crystal/codegen/call.cr b/src/compiler/crystal/codegen/call.cr
@@ -54,6 +54,8 @@ class Crystal::CodeGenVisitor
   end
 
   def prepare_call_args_non_external(node, target_def, owner)
+    is_primitive = target_def.body.is_a?(Primitive)
+
     call_args = Array(LLVM::Value).new(node.args.size + 1)
     old_needs_value = @needs_value
 
@@ -83,6 +85,8 @@ class Crystal::CodeGenVisitor
       end
     end
 
+    c_calling_convention = target_def.c_calling_convention?
+
     # Then the arguments.
     node.args.zip(target_def.args) do |arg, def_arg|
       @needs_value = true
@@ -95,6 +99,13 @@ class Crystal::CodeGenVisitor
         call_arg = llvm_nil if arg.type.nil_type?
         call_arg = downcast(call_arg, def_arg.type, arg.type, true)
       end
+
+      # - C calling convention passing needs a separate handling of pass-by-value
+      # - Primitives might need a separate handling (for example invoking a Proc)
+      if arg.type.passed_by_value? && !c_calling_convention && !is_primitive
+        call_arg = load(call_arg)
+      end
+
       call_args << call_arg
     end
 
@@ -503,18 +514,7 @@ class Crystal::CodeGenVisitor
     if external = target_def.c_calling_convention?
       set_call_attributes_external(node, external)
     else
-      set_call_attributes_non_external(node, target_def, self_type, is_closure, fun_type)
-    end
-  end
-
-  def set_call_attributes_non_external(node, target_def, self_type, is_closure, fun_type)
-    arg_offset = 1
-    arg_offset += 1 if self_type.try(&.passed_as_self?)
-
-    node.args.each_with_index do |arg, i|
-      next unless arg.type.passed_by_value?
-
-      @last.add_instruction_attribute(i + arg_offset, LLVM::Attribute::ByVal)
+      # Non-external methods/functions have no arguments attributes
     end
   end
 
@@ -530,16 +530,8 @@ class Crystal::CodeGenVisitor
       next if node.args[i]?.try &.is_a?(Out)
 
       abi_arg_type = abi_info.arg_types[i]?
-      if abi_arg_type
-        if (attr = abi_arg_type.attr)
-          @last.add_instruction_attribute(i + arg_offset, attr)
-        end
-      else
-        # TODO: this is for variadic arguments, which is still not handled properly (in regards to the ABI for structs)
-        arg_type = arg.type
-        next unless arg_type.passed_by_value?
-
-        @last.add_instruction_attribute(i + arg_offset, LLVM::Attribute::ByVal)
+      if abi_arg_type && (attr = abi_arg_type.attr)
+        @last.add_instruction_attribute(i + arg_offset, attr)
       end
     end
 
@@ -555,13 +547,8 @@ class Crystal::CodeGenVisitor
     arg_offset = is_closure ? 2 : 1
     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]?)
-        if (attr = abi_arg_type.attr)
-          @last.add_instruction_attribute(i + arg_offset, attr)
-        end
-      else
-        next unless arg_type.passed_by_value?
-        @last.add_instruction_attribute(i + arg_offset, LLVM::Attribute::ByVal)
+      if abi_info && (abi_arg_type = abi_info.arg_types[i]?) && (attr = abi_arg_type.attr)
+        @last.add_instruction_attribute(i + arg_offset, attr)
       end
     end
   end

diff --git a/src/compiler/crystal/codegen/cast.cr b/src/compiler/crystal/codegen/cast.cr
@@ -69,7 +69,7 @@ require "./codegen"
 # type into a specific type (such as when casting a Foo to a Bar, with Bar < Foo).
 
 class Crystal::CodeGenVisitor
-  def assign(target_pointer, target_type, value_type, value)
+  def assign(target_pointer, target_type, value_type, value, already_loaded = false)
     return if @builder.end
 
     target_type = target_type.remove_indirection
@@ -79,34 +79,35 @@ class Crystal::CodeGenVisitor
       if target_type.nil_type?
         value
       else
-        store to_rhs(value, target_type), target_pointer
+        value = to_rhs(value, target_type) unless already_loaded
+        store value, target_pointer
       end
     else
-      assign_distinct target_pointer, target_type, value_type, value
+      assign_distinct target_pointer, target_type, value_type, value, already_loaded
     end
   end
 
-  def assign_distinct(target_pointer, target_type : NilableType, value_type : Type, value)
+  def assign_distinct(target_pointer, target_type : NilableType, value_type : Type, value, already_loaded)
     store upcast(value, target_type, value_type), target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : ReferenceUnionType, value_type : ReferenceUnionType, value)
+  def assign_distinct(target_pointer, target_type : ReferenceUnionType, value_type : ReferenceUnionType, value, already_loaded)
     store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : ReferenceUnionType, value_type : VirtualType, value)
+  def assign_distinct(target_pointer, target_type : ReferenceUnionType, value_type : VirtualType, value, already_loaded)
     store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : ReferenceUnionType, value_type : Type, value)
+  def assign_distinct(target_pointer, target_type : ReferenceUnionType, value_type : Type, value, already_loaded)
     store cast_to(value, target_type), target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : NilableReferenceUnionType, value_type : Type, value)
+  def assign_distinct(target_pointer, target_type : NilableReferenceUnionType, value_type : Type, value, already_loaded)
     store upcast(value, target_type, value_type), target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : MixedUnionType, value)
+  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : MixedUnionType, value, already_loaded)
     # It might happen that some types inside the union `value_type` are not inside `target_type`,
     # for example with named tuple of same keys with different order. In that case we need cast
     # those value to the correct type before finally storing them in the target union.
@@ -169,23 +170,23 @@ class Crystal::CodeGenVisitor
     store load(casted_value), target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : NilableType, value)
+  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : NilableType, value, already_loaded)
     store_in_union target_pointer, value_type, value
   end
 
-  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : VoidType, value)
+  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : VoidType, value, already_loaded)
     store type_id(value_type), union_type_id(target_pointer)
   end
 
-  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : BoolType, value)
+  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : BoolType, value, already_loaded)
     store_bool_in_union target_type, target_pointer, value
   end
 
-  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : NilType, value)
+  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : NilType, value, already_loaded)
     store_nil_in_union target_pointer, target_type
   end
 
-  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : Type, value)
+  def assign_distinct(target_pointer, target_type : MixedUnionType, value_type : Type, value, already_loaded)
     case value_type
     when TupleInstanceType, NamedTupleInstanceType
       # It might happen that `value_type` is not of the union but it's compatible with one of them.
@@ -197,52 +198,53 @@ class Crystal::CodeGenVisitor
       end
     end
 
-    store_in_union target_pointer, value_type, to_rhs(value, value_type)
+    value = to_rhs(value, value_type) unless already_loaded
+    store_in_union target_pointer, value_type, value
   end
 
-  def assign_distinct(target_pointer, target_type : VirtualType, value_type : MixedUnionType, value)
+  def assign_distinct(target_pointer, target_type : VirtualType, value_type : MixedUnionType, value, already_loaded)
     casted_value = cast_to_pointer(union_value(value), target_type)
     store load(casted_value), target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : VirtualType, value_type : Type, value)
+  def assign_distinct(target_pointer, target_type : VirtualType, value_type : Type, value, already_loaded)
     store cast_to(value, target_type), target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : VirtualMetaclassType, value_type : MetaclassType, value)
+  def assign_distinct(target_pointer, target_type : VirtualMetaclassType, value_type : MetaclassType, value, already_loaded)
     store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : VirtualMetaclassType, value_type : VirtualMetaclassType, value)
+  def assign_distinct(target_pointer, target_type : VirtualMetaclassType, value_type : VirtualMetaclassType, value, already_loaded)
     store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : NilableProcType, value_type : NilType, value)
+  def assign_distinct(target_pointer, target_type : NilableProcType, value_type : NilType, value, already_loaded)
     nilable_fun = make_nilable_fun target_type
     store nilable_fun, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : NilableProcType, value_type : ProcInstanceType, value)
+  def assign_distinct(target_pointer, target_type : NilableProcType, value_type : ProcInstanceType, value, already_loaded)
     store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : NilableProcType, value_type : TypeDefType, value)
-    assign_distinct target_pointer, target_type, value_type.typedef, value
+  def assign_distinct(target_pointer, target_type : NilableProcType, value_type : TypeDefType, value, already_loaded)
+    assign_distinct target_pointer, target_type, value_type.typedef, value, already_loaded
   end
 
-  def assign_distinct(target_pointer, target_type : NilablePointerType, value_type : NilType, value)
+  def assign_distinct(target_pointer, target_type : NilablePointerType, value_type : NilType, value, already_loaded)
     store llvm_type(target_type).null, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : NilablePointerType, value_type : PointerInstanceType, value)
+  def assign_distinct(target_pointer, target_type : NilablePointerType, value_type : PointerInstanceType, value, already_loaded)
     store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : NilablePointerType, value_type : TypeDefType, value)
-    assign_distinct target_pointer, target_type, value_type.typedef, value
+  def assign_distinct(target_pointer, target_type : NilablePointerType, value_type : TypeDefType, value, already_loaded)
+    assign_distinct target_pointer, target_type, value_type.typedef, value, already_loaded
   end
 
-  def assign_distinct(target_pointer, target_type : TupleInstanceType, value_type : TupleInstanceType, value)
+  def assign_distinct(target_pointer, target_type : TupleInstanceType, value_type : TupleInstanceType, value, already_loaded)
     index = 0
     target_type.tuple_types.zip(value_type.tuple_types) do |target_tuple_type, value_tuple_type|
       target_ptr = gep target_pointer, 0, index
@@ -254,7 +256,7 @@ class Crystal::CodeGenVisitor
     value
   end
 
-  def assign_distinct(target_pointer, target_type : NamedTupleInstanceType, value_type : NamedTupleInstanceType, value)
+  def assign_distinct(target_pointer, target_type : NamedTupleInstanceType, value_type : NamedTupleInstanceType, value, already_loaded)
     value_type.entries.each_with_index do |entry, index|
       value_ptr = aggregate_index(value, index)
       value_at_index = to_lhs(value_ptr, entry.type)
@@ -264,12 +266,13 @@ class Crystal::CodeGenVisitor
     end
   end
 
-  def assign_distinct(target_pointer, target_type : ProcInstanceType, value_type : ProcInstanceType, value)
+  def assign_distinct(target_pointer, target_type : ProcInstanceType, value_type : ProcInstanceType, value, already_loaded)
     # Cast of a non-void proc to a void proc
-    store to_rhs(value, target_type), target_pointer
+    value = to_rhs(value, target_type) unless already_loaded
+    store value, target_pointer
   end
 
-  def assign_distinct(target_pointer, target_type : Type, value_type : Type, value)
+  def assign_distinct(target_pointer, target_type : Type, value_type : Type, value, already_loaded)
     raise "Bug: trying to assign #{target_type} <- #{value_type}"
   end
 

diff --git a/src/compiler/crystal/codegen/codegen.cr b/src/compiler/crystal/codegen/codegen.cr
@@ -909,17 +909,7 @@ module Crystal
         llvm_value = check_proc_is_not_closure(llvm_value, target.type)
       end
 
-      if target.is_a?(Var) && target.special_var? && !target_type.reference_like?
-        # For special vars that are not reference-like, the function argument will
-        # be a pointer to the struct value. So, we need to first cast the value to
-        # that type (without the pointer), load it, and store it in the argument.
-        # If it's a reference-like then it's just a pointer and we can reuse the
-        # logic in the other branch.
-        llvm_value = upcast llvm_value, target_type, value.type
-        store load(llvm_value), ptr
-      else
-        assign ptr, target_type, value.type, llvm_value
-      end
+      assign ptr, target_type, value.type, llvm_value
 
       false
     end

diff --git a/src/compiler/crystal/codegen/fun.cr b/src/compiler/crystal/codegen/fun.cr
@@ -191,16 +191,23 @@ class Crystal::CodeGenVisitor
       args.push Arg.new(special_var_name, type: target_def.vars.not_nil![special_var_name].type)
     end
 
-    llvm_args_types = args.map do |arg|
+    llvm_args_types = args.map_with_index do |arg, i|
       arg_type = arg.type
       if arg_type.void?
         llvm_arg_type = LLVM::Int8
       else
-        llvm_arg_type = llvm_arg_type(arg_type)
-        # We need an extra pointer for special vars that are reference-like:
-        # non-reference-like are unions, so their argument representation is already a pointer
-        # (passed byval, but for these we don't pass them byval)
-        llvm_arg_type = llvm_arg_type.pointer if arg.special_var? && arg_type.reference_like?
+        llvm_arg_type = llvm_type(arg_type)
+
+        # We need an extra pointer for special vars (they always have an extra pointer)
+        if arg.special_var?
+          llvm_arg_type = llvm_arg_type.pointer
+        end
+
+        # Self is always passed by reference (pointer),
+        # even if the type is passed by value (like a struct)
+        if i == 0 && !is_fun_literal && self_type.passed_as_self? && self_type.passed_by_value?
+          llvm_arg_type = llvm_arg_type.pointer
+        end
       end
       llvm_arg_type
     end
@@ -222,15 +229,6 @@ class Crystal::CodeGenVisitor
     args.each_with_index do |arg, i|
       param = context.fun.params[i + offset]
       param.name = arg.name
-
-      # Set 'byval' attribute
-      # but don't set it if it's the "self" argument and it's a struct (while not in a closure).
-      if arg.type.passed_by_value?
-        if ((is_fun_literal && !is_closure) || (is_closure || !(i == 0 && self_type.struct?))) &&
-           !arg.special_var? # special vars are never passed byval
-          param.add_attribute LLVM::Attribute::ByVal
-        end
-      end
     end
 
     args
@@ -401,8 +399,16 @@ class Crystal::CodeGenVisitor
     var_type = (target_def_var || arg).type
     return if var_type.void?
 
+    # Pass-by-value parameters are passed as-is, so they are
+    # "already loaded" (they are not behind a pointer)
+    already_loaded = false
+
     if closure_var = context.vars[arg.name]?
       pointer = closure_var.pointer
+
+      if var_type.passed_by_value?
+        already_loaded = true
+      end
     else
       # 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
@@ -412,21 +418,37 @@ class Crystal::CodeGenVisitor
         store value, casted_pointer
         context.vars[arg.name] = LLVMVar.new(pointer, var_type)
         return
+      elsif arg.special_var?
+        context.vars[arg.name] = LLVMVar.new(value, var_type)
+        return
       else
         # We don't need to create a copy of the argument if it's never
         # assigned a value inside the function.
         needs_copy = target_def_var.try &.assigned_to?
-        if needs_copy && !arg.special_var?
+        if needs_copy
           pointer = alloca(llvm_type(var_type), arg.name)
           context.vars[arg.name] = LLVMVar.new(pointer, var_type)
+
+          if arg.type.passed_by_value? && !value.attributes.by_val?
+            already_loaded = true
+          end
         else
-          context.vars[arg.name] = LLVMVar.new(value, var_type, true)
-          return
+          if arg.type.passed_by_value? && !value.attributes.by_val?
+            # For pass-by-value we create an alloca so the value
+            # is behind a pointer, as everywhere else
+            pointer = alloca(llvm_type(var_type), arg.name)
+            store value, pointer
+            context.vars[arg.name] = LLVMVar.new(pointer, var_type)
+            return
+          else
+            context.vars[arg.name] = LLVMVar.new(value, var_type, true)
+            return
+          end
         end
       end
     end
 
-    assign pointer, var_type, arg.type, value
+    assign pointer, var_type, arg.type, value, already_loaded
   end
 
   def type_module(type)

diff --git a/src/compiler/crystal/codegen/llvm_typer.cr b/src/compiler/crystal/codegen/llvm_typer.cr
@@ -359,18 +359,6 @@ module Crystal
       raise "Bug: called llvm_struct_type for #{type}"
     end
 
-    def llvm_arg_type(type : AliasType)
-      llvm_arg_type(type.remove_alias)
-    end
-
-    def llvm_arg_type(type : Type)
-      if type.passed_by_value?
-        llvm_type(type).pointer
-      else
-        llvm_type(type)
-      end
-    end
-
     def llvm_embedded_type(type, wants_size = false)
       type = type.remove_indirection
       case type
@@ -404,8 +392,12 @@ module Crystal
     def llvm_c_type(type)
       if type.extern?
         llvm_struct_type(type)
+      elsif type.passed_by_value?
+        # C types that are passed by value must be considered,
+        # for the ABI, as being passed behind a pointer
+        llvm_type(type).pointer
       else
-        llvm_arg_type(type)
+        llvm_type(type)
       end
     end
 
@@ -426,13 +418,13 @@ module Crystal
     end
 
     def closure_type(type : ProcInstanceType)
-      arg_types = type.arg_types.map { |arg_type| llvm_arg_type(arg_type) }
+      arg_types = type.arg_types.map { |arg_type| llvm_type(arg_type) }
       arg_types.insert(0, LLVM::VoidPointer)
       LLVM::Type.function(arg_types, llvm_type(type.return_type)).pointer
     end
 
     def proc_type(type : ProcInstanceType)
-      arg_types = type.arg_types.map { |arg_type| llvm_arg_type(arg_type).as(LLVM::Type) }
+      arg_types = type.arg_types.map { |arg_type| llvm_type(arg_type).as(LLVM::Type) }
       LLVM::Type.function(arg_types, llvm_type(type.return_type)).pointer
     end
 

diff --git a/src/compiler/crystal/codegen/phi.cr b/src/compiler/crystal/codegen/phi.cr
@@ -89,7 +89,10 @@ class Crystal::CodeGenVisitor
           @codegen.last = llvm_nil
         else
           if @exit_block
-            @codegen.last = phi llvm_arg_type(@node_type.not_nil!), phi_table
+            node_type = @node_type.not_nil!
+            type = llvm_type(node_type)
+            type = type.pointer if node_type.passed_by_value?
+            @codegen.last = phi type, phi_table
           else
             @codegen.last = phi_table.values.first
           end

diff --git a/src/compiler/crystal/codegen/primitives.cr b/src/compiler/crystal/codegen/primitives.cr
@@ -618,14 +618,26 @@ class Crystal::CodeGenVisitor
 
   def codegen_primitive_proc_call(node, target_def, call_args)
     closure_ptr = call_args[0]
+
+    # For non-closure args we use byval attribute and other things
+    # that the C ABI dictates, if needed (args).
+    # Otherwise we load the values (closure_args).
     args = call_args[1..-1]
+    closure_args = Array(LLVM::Value).new(args.size + 1)
+
+    c_calling_convention = target_def.proc_c_calling_convention?
 
     proc_type = context.type.as(ProcInstanceType)
     0.upto(target_def.args.size - 1) do |i|
       arg = args[i]
       proc_arg_type = proc_type.arg_types[i]
       target_def_arg_type = target_def.args[i].type
       args[i] = upcast arg, proc_arg_type, target_def_arg_type
+      if proc_arg_type.passed_by_value?
+        closure_args << load(args[i])
+      else
+        closure_args << args[i]
+      end
     end
 
     fun_ptr = builder.extract_value closure_ptr, 0
@@ -650,10 +662,10 @@ class Crystal::CodeGenVisitor
       # arguments according to the ABI.
       # For this we temporarily set the target_def's `abi_info` and `c_calling_convention`
       # properties for the non-closure branch, and then reset it.
-      if target_def.proc_c_calling_convention?
+      if c_calling_convention
         null_fun_ptr, null_args = codegen_extern_primitive_proc_call(target_def, args, fun_ptr)
       else
-        null_fun_ptr, null_args = real_fun_ptr, args
+        null_fun_ptr, null_args = real_fun_ptr, closure_args
       end
 
       value = codegen_call_or_invoke(node, target_def, nil, null_fun_ptr, null_args, true, target_def.type, false, proc_type)
@@ -665,8 +677,8 @@ class Crystal::CodeGenVisitor
 
       position_at_end ctx_is_not_null_block
       real_fun_ptr = bit_cast fun_ptr, llvm_closure_type(context.type)
-      args.insert(0, ctx_ptr)
-      value = codegen_call_or_invoke(node, target_def, nil, real_fun_ptr, args, true, target_def.type, true, proc_type)
+      closure_args.insert(0, ctx_ptr)
+      value = codegen_call_or_invoke(node, target_def, nil, real_fun_ptr, closure_args, true, target_def.type, true, proc_type)
       phi.add value, node.type, true
     end