From 65bc4d915daf33255f16a1586e69040fd9148e87 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Mon, 18 Jul 2022 19:55:55 -0700 Subject: [PATCH 1/4] LLVM: lower optional types as byref=true This is a possible workaround for https://github.com/llvm/llvm-project/issues/56585 On my computer it makes stage3-release go from false positive compilation errors on the behavior tests to "segmentation fault". Is this forwards progress or backwards progress? I have no idea. See #11450 --- src/codegen/llvm.zig | 98 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 84 insertions(+), 14 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index d34bdea7660f..61f6eff441d9 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -4539,7 +4539,7 @@ pub const FuncGen = struct { } // We need to emit instructions to check for equality/inequality // of optionals that are not pointers. - const is_by_ref = isByRef(operand_ty); + const is_by_ref = isByRef(scalar_ty); const lhs_non_null = self.optIsNonNull(lhs, is_by_ref); const rhs_non_null = self.optIsNonNull(rhs, is_by_ref); const llvm_i2 = self.context.intType(2); @@ -4564,8 +4564,8 @@ pub const FuncGen = struct { _ = self.builder.buildBr(end_block); self.builder.positionBuilderAtEnd(both_pl_block); - const lhs_payload = self.optPayloadHandle(lhs, is_by_ref); - const rhs_payload = self.optPayloadHandle(rhs, is_by_ref); + const lhs_payload = self.optPayloadHandle(lhs, scalar_ty); + const rhs_payload = self.optPayloadHandle(rhs, scalar_ty); const payload_cmp = try self.cmp(lhs_payload, rhs_payload, payload_ty, op); _ = self.builder.buildBr(end_block); const both_pl_block_end = self.builder.getInsertBlock(); @@ -5795,7 +5795,7 @@ pub const FuncGen = struct { return operand; } - return self.optPayloadHandle(operand, isByRef(payload_ty)); + return self.optPayloadHandle(operand, optional_ty); } fn airErrUnionPayload( @@ -7347,11 +7347,9 @@ pub const FuncGen = struct { } comptime assert(optional_layout_version == 3); - const optional_llvm_ty = try self.dg.lowerType(optional_ty); + const non_null_bit = self.builder.buildNot(success_bit, ""); - const non_null_field = self.builder.buildZExt(non_null_bit, self.dg.context.intType(8), ""); - const partial = self.builder.buildInsertValue(optional_llvm_ty.getUndef(), payload, 0, ""); - return self.builder.buildInsertValue(partial, non_null_field, 1, ""); + return buildOptional(self, optional_ty, payload, non_null_bit); } fn airAtomicRmw(self: *FuncGen, inst: Air.Inst.Index) !?*const llvm.Value { @@ -7515,11 +7513,13 @@ pub const FuncGen = struct { const union_ptr = try self.resolveInst(bin_op.lhs); const new_tag = try self.resolveInst(bin_op.rhs); if (layout.payload_size == 0) { + // TODO alignment on this store _ = self.builder.buildStore(new_tag, union_ptr); return null; } const tag_index = @boolToInt(layout.tag_align < layout.payload_align); const tag_field_ptr = self.builder.buildStructGEP(union_ptr, tag_index, ""); + // TODO alignment on this store _ = self.builder.buildStore(new_tag, tag_field_ptr); return null; } @@ -8355,18 +8355,77 @@ pub const FuncGen = struct { } /// Assumes the optional is not pointer-like and payload has bits. - fn optPayloadHandle(self: *FuncGen, opt_handle: *const llvm.Value, is_by_ref: bool) *const llvm.Value { - if (is_by_ref) { + fn optPayloadHandle( + fg: *FuncGen, + opt_handle: *const llvm.Value, + opt_ty: Type, + ) *const llvm.Value { + var buf: Type.Payload.ElemType = undefined; + const payload_ty = opt_ty.optionalChild(&buf); + + if (isByRef(opt_ty)) { // We have a pointer and we need to return a pointer to the first field. - const index_type = self.context.intType(32); + const index_type = fg.context.intType(32); const indices: [2]*const llvm.Value = .{ index_type.constNull(), // dereference the pointer index_type.constNull(), // first field is the payload }; - return self.builder.buildInBoundsGEP(opt_handle, &indices, indices.len, ""); + const payload_ptr = fg.builder.buildInBoundsGEP(opt_handle, &indices, indices.len, ""); + + if (isByRef(payload_ty)) { + return payload_ptr; + } + const target = fg.dg.module.getTarget(); + const payload_alignment = payload_ty.abiAlignment(target); + const load_inst = fg.builder.buildLoad(payload_ptr, ""); + load_inst.setAlignment(payload_alignment); + return load_inst; + } + + assert(!isByRef(payload_ty)); + return fg.builder.buildExtractValue(opt_handle, 0, ""); + } + + fn buildOptional( + self: *FuncGen, + optional_ty: Type, + payload: *const llvm.Value, + non_null_bit: *const llvm.Value, + ) !?*const llvm.Value { + const optional_llvm_ty = try self.dg.lowerType(optional_ty); + const non_null_field = self.builder.buildZExt(non_null_bit, self.dg.context.intType(8), ""); + + if (isByRef(optional_ty)) { + const target = self.dg.module.getTarget(); + const alloca_inst = self.buildAlloca(optional_llvm_ty); + const payload_alignment = optional_ty.abiAlignment(target); + alloca_inst.setAlignment(payload_alignment); + + const index_type = self.context.intType(32); + { + const indices: [2]*const llvm.Value = .{ + index_type.constNull(), // dereference the pointer + index_type.constNull(), // first field is the payload + }; + const field_ptr = self.builder.buildInBoundsGEP(alloca_inst, &indices, indices.len, ""); + const store_inst = self.builder.buildStore(payload, field_ptr); + store_inst.setAlignment(payload_alignment); + } + { + const indices: [2]*const llvm.Value = .{ + index_type.constNull(), // dereference the pointer + index_type.constInt(1, .False), // second field is the non-null bit + }; + const field_ptr = self.builder.buildInBoundsGEP(alloca_inst, &indices, indices.len, ""); + const store_inst = self.builder.buildStore(non_null_field, field_ptr); + store_inst.setAlignment(1); + } + + return alloca_inst; } - return self.builder.buildExtractValue(opt_handle, 0, ""); + const partial = self.builder.buildInsertValue(optional_llvm_ty.getUndef(), payload, 0, ""); + return self.builder.buildInsertValue(partial, non_null_field, 1, ""); } fn fieldPtr( @@ -9335,7 +9394,18 @@ fn isByRef(ty: Type) bool { .ErrorUnion => return isByRef(ty.errorUnionPayload()), .Optional => { var buf: Type.Payload.ElemType = undefined; - return isByRef(ty.optionalChild(&buf)); + const payload_ty = ty.optionalChild(&buf); + if (!payload_ty.hasRuntimeBitsIgnoreComptime()) { + return false; + } + if (ty.optionalReprIsPayload()) { + return false; + } + return true; + // TODO we actually want this logic: + // however it is tripping an LLVM 14 regression: + // https://github.com/llvm/llvm-project/issues/56585 + //return isByRef(payload_ty); }, } } From bab570a22540c2acb15c19e1d080a80d355407e3 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Mon, 18 Jul 2022 20:59:39 -0700 Subject: [PATCH 2/4] LLVM: lower all structs as byref=true Same reasoning as previous commit. --- src/codegen/llvm.zig | 127 ++++++++++++++++++++++--------------------- 1 file changed, 64 insertions(+), 63 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 61f6eff441d9..5f1b42cea3ce 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -5730,12 +5730,7 @@ pub const FuncGen = struct { // The payload and the optional are the same value. return operand; } - const index_type = self.context.intType(32); - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constNull(), // first field is the payload - }; - return self.builder.buildInBoundsGEP(operand, &indices, indices.len, ""); + return self.builder.buildStructGEP(operand, 0, ""); } fn airOptionalPayloadPtrSet(self: *FuncGen, inst: Air.Inst.Index) !?*const llvm.Value { @@ -5761,24 +5756,17 @@ pub const FuncGen = struct { // Setting to non-null will be done when the payload is set. return operand; } - const index_type = self.context.intType(32); - { - // First set the non-null bit. - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constInt(1, .False), // second field is the non-null bit - }; - const non_null_ptr = self.builder.buildInBoundsGEP(operand, &indices, indices.len, ""); - _ = self.builder.buildStore(non_null_bit, non_null_ptr); - } + + // First set the non-null bit. + const non_null_ptr = self.builder.buildStructGEP(operand, 1, ""); + // TODO set alignment on this store + _ = self.builder.buildStore(non_null_bit, non_null_ptr); + // Then return the payload pointer (only if it's used). if (self.liveness.isUnused(inst)) return null; - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constNull(), // first field is the payload - }; - return self.builder.buildInBoundsGEP(operand, &indices, indices.len, ""); + + return self.builder.buildStructGEP(operand, 0, ""); } fn airOptionalPayload(self: *FuncGen, inst: Air.Inst.Index) !?*const llvm.Value { @@ -5874,16 +5862,11 @@ pub const FuncGen = struct { _ = self.builder.buildStore(non_error_val, operand); return operand; } - const index_type = self.context.intType(32); const target = self.dg.module.getTarget(); { const error_offset = errUnionErrorOffset(payload_ty, target); // First set the non-error value. - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constInt(error_offset, .False), - }; - const non_null_ptr = self.builder.buildInBoundsGEP(operand, &indices, indices.len, ""); + const non_null_ptr = self.builder.buildStructGEP(operand, error_offset, ""); const store_inst = self.builder.buildStore(non_error_val, non_null_ptr); store_inst.setAlignment(Type.anyerror.abiAlignment(target)); } @@ -5892,11 +5875,7 @@ pub const FuncGen = struct { return null; const payload_offset = errUnionPayloadOffset(payload_ty, target); - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constInt(payload_offset, .False), - }; - return self.builder.buildInBoundsGEP(operand, &indices, indices.len, ""); + return self.builder.buildStructGEP(operand, payload_offset, ""); } fn airErrReturnTrace(self: *FuncGen, _: Air.Inst.Index) !?*const llvm.Value { @@ -6391,8 +6370,30 @@ pub const FuncGen = struct { const overflow_bit = self.builder.buildExtractValue(result_struct, 1, ""); var ty_buf: Type.Payload.Pointer = undefined; - const partial = self.builder.buildInsertValue(llvm_dest_ty.getUndef(), result, llvmFieldIndex(dest_ty, 0, tg, &ty_buf).?, ""); - return self.builder.buildInsertValue(partial, overflow_bit, llvmFieldIndex(dest_ty, 1, tg, &ty_buf).?, ""); + const result_index = llvmFieldIndex(dest_ty, 0, tg, &ty_buf).?; + const overflow_index = llvmFieldIndex(dest_ty, 1, tg, &ty_buf).?; + + if (isByRef(dest_ty)) { + const target = self.dg.module.getTarget(); + const alloca_inst = self.buildAlloca(llvm_dest_ty); + const result_alignment = dest_ty.abiAlignment(target); + alloca_inst.setAlignment(result_alignment); + { + const field_ptr = self.builder.buildStructGEP(alloca_inst, result_index, ""); + const store_inst = self.builder.buildStore(result, field_ptr); + store_inst.setAlignment(result_alignment); + } + { + const field_ptr = self.builder.buildStructGEP(alloca_inst, overflow_index, ""); + const store_inst = self.builder.buildStore(overflow_bit, field_ptr); + store_inst.setAlignment(1); + } + + return alloca_inst; + } + + const partial = self.builder.buildInsertValue(llvm_dest_ty.getUndef(), result, result_index, ""); + return self.builder.buildInsertValue(partial, overflow_bit, overflow_index, ""); } fn buildElementwiseCall( @@ -6721,8 +6722,30 @@ pub const FuncGen = struct { const overflow_bit = self.builder.buildICmp(.NE, lhs, reconstructed, ""); var ty_buf: Type.Payload.Pointer = undefined; - const partial = self.builder.buildInsertValue(llvm_dest_ty.getUndef(), result, llvmFieldIndex(dest_ty, 0, tg, &ty_buf).?, ""); - return self.builder.buildInsertValue(partial, overflow_bit, llvmFieldIndex(dest_ty, 1, tg, &ty_buf).?, ""); + const result_index = llvmFieldIndex(dest_ty, 0, tg, &ty_buf).?; + const overflow_index = llvmFieldIndex(dest_ty, 1, tg, &ty_buf).?; + + if (isByRef(dest_ty)) { + const target = self.dg.module.getTarget(); + const alloca_inst = self.buildAlloca(llvm_dest_ty); + const result_alignment = dest_ty.abiAlignment(target); + alloca_inst.setAlignment(result_alignment); + { + const field_ptr = self.builder.buildStructGEP(alloca_inst, result_index, ""); + const store_inst = self.builder.buildStore(result, field_ptr); + store_inst.setAlignment(result_alignment); + } + { + const field_ptr = self.builder.buildStructGEP(alloca_inst, overflow_index, ""); + const store_inst = self.builder.buildStore(overflow_bit, field_ptr); + store_inst.setAlignment(1); + } + + return alloca_inst; + } + + const partial = self.builder.buildInsertValue(llvm_dest_ty.getUndef(), result, result_index, ""); + return self.builder.buildInsertValue(partial, overflow_bit, overflow_index, ""); } fn airAnd(self: *FuncGen, inst: Air.Inst.Index) !?*const llvm.Value { @@ -8336,17 +8359,9 @@ pub const FuncGen = struct { fn optIsNonNull(self: *FuncGen, opt_handle: *const llvm.Value, is_by_ref: bool) *const llvm.Value { const field = b: { if (is_by_ref) { - const index_type = self.context.intType(32); - - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), - index_type.constInt(1, .False), - }; - - const field_ptr = self.builder.buildInBoundsGEP(opt_handle, &indices, indices.len, ""); + const field_ptr = self.builder.buildStructGEP(opt_handle, 1, ""); break :b self.builder.buildLoad(field_ptr, ""); } - break :b self.builder.buildExtractValue(opt_handle, 1, ""); }; comptime assert(optional_layout_version == 3); @@ -8365,12 +8380,7 @@ pub const FuncGen = struct { if (isByRef(opt_ty)) { // We have a pointer and we need to return a pointer to the first field. - const index_type = fg.context.intType(32); - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constNull(), // first field is the payload - }; - const payload_ptr = fg.builder.buildInBoundsGEP(opt_handle, &indices, indices.len, ""); + const payload_ptr = fg.builder.buildStructGEP(opt_handle, 0, ""); if (isByRef(payload_ty)) { return payload_ptr; @@ -8401,22 +8411,13 @@ pub const FuncGen = struct { const payload_alignment = optional_ty.abiAlignment(target); alloca_inst.setAlignment(payload_alignment); - const index_type = self.context.intType(32); { - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constNull(), // first field is the payload - }; - const field_ptr = self.builder.buildInBoundsGEP(alloca_inst, &indices, indices.len, ""); + const field_ptr = self.builder.buildStructGEP(alloca_inst, 0, ""); const store_inst = self.builder.buildStore(payload, field_ptr); store_inst.setAlignment(payload_alignment); } { - const indices: [2]*const llvm.Value = .{ - index_type.constNull(), // dereference the pointer - index_type.constInt(1, .False), // second field is the non-null bit - }; - const field_ptr = self.builder.buildInBoundsGEP(alloca_inst, &indices, indices.len, ""); + const field_ptr = self.builder.buildStructGEP(alloca_inst, 1, ""); const store_inst = self.builder.buildStore(non_null_field, field_ptr); store_inst.setAlignment(1); } @@ -9337,7 +9338,7 @@ fn ccAbiPromoteInt( fn isByRef(ty: Type) bool { // For tuples and structs, if there are more than this many non-void // fields, then we make it byref, otherwise byval. - const max_fields_byval = 2; + const max_fields_byval = 0; switch (ty.zigTypeTag()) { .Type, From 74fb65fb424a0fbb2eb00109fe4cee17aa2646c2 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Mon, 18 Jul 2022 21:27:40 -0700 Subject: [PATCH 3/4] LLVM: lower all error unions as byref=true Same reasoning as previous commit. --- src/codegen/llvm.zig | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 5f1b42cea3ce..992ea0ff45e3 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -4740,12 +4740,14 @@ pub const FuncGen = struct { const err_set_ty = try fg.dg.lowerType(Type.anyerror); const zero = err_set_ty.constNull(); if (!payload_has_bits) { + // TODO add alignment to this load const loaded = if (operand_is_ptr) fg.builder.buildLoad(err_union, "") else err_union; break :err fg.builder.buildICmp(.NE, loaded, zero, ""); } const err_field_index = errUnionErrorOffset(payload_ty, target); if (operand_is_ptr or isByRef(err_union_ty)) { const err_field_ptr = fg.builder.buildStructGEP(err_union, err_field_index, ""); + // TODO add alignment to this load const loaded = fg.builder.buildLoad(err_field_ptr, ""); break :err fg.builder.buildICmp(.NE, loaded, zero, ""); } @@ -4765,13 +4767,22 @@ pub const FuncGen = struct { if (!payload_has_bits) { if (!operand_is_ptr) return null; - // TODO once we update to LLVM 14 this bitcast won't be necessary. + // TODO once we update to an LLVM version with opaque pointers + // this bitcast won't be necessary. const res_ptr_ty = try fg.dg.lowerType(result_ty); return fg.builder.buildBitCast(err_union, res_ptr_ty, ""); } const offset = errUnionPayloadOffset(payload_ty, target); if (operand_is_ptr or isByRef(payload_ty)) { return fg.builder.buildStructGEP(err_union, offset, ""); + } else if (isByRef(err_union_ty)) { + const payload_ptr = fg.builder.buildStructGEP(err_union, offset, ""); + if (isByRef(payload_ty)) { + return payload_ptr; + } + const load_inst = fg.builder.buildLoad(payload_ptr, ""); + load_inst.setAlignment(payload_ty.abiAlignment(target)); + return load_inst; } return fg.builder.buildExtractValue(err_union, offset, ""); } @@ -5795,6 +5806,8 @@ pub const FuncGen = struct { const ty_op = self.air.instructions.items(.data)[inst].ty_op; const operand = try self.resolveInst(ty_op.operand); + const operand_ty = self.air.typeOf(ty_op.operand); + const err_union_ty = if (operand_is_ptr) operand_ty.childType() else operand_ty; const result_ty = self.air.typeOfIndex(inst); const payload_ty = if (operand_is_ptr) result_ty.childType() else result_ty; const target = self.dg.module.getTarget(); @@ -5809,6 +5822,14 @@ pub const FuncGen = struct { const offset = errUnionPayloadOffset(payload_ty, target); if (operand_is_ptr or isByRef(payload_ty)) { return self.builder.buildStructGEP(operand, offset, ""); + } else if (isByRef(err_union_ty)) { + const payload_ptr = self.builder.buildStructGEP(operand, offset, ""); + if (isByRef(payload_ty)) { + return payload_ptr; + } + const load_inst = self.builder.buildLoad(payload_ptr, ""); + load_inst.setAlignment(payload_ty.abiAlignment(target)); + return load_inst; } return self.builder.buildExtractValue(operand, offset, ""); } @@ -9338,6 +9359,8 @@ fn ccAbiPromoteInt( fn isByRef(ty: Type) bool { // For tuples and structs, if there are more than this many non-void // fields, then we make it byref, otherwise byval. + // TODO we actually want to set this to 2, however it is tripping an LLVM 14 regression: + // https://github.com/llvm/llvm-project/issues/56585 const max_fields_byval = 0; switch (ty.zigTypeTag()) { @@ -9392,7 +9415,17 @@ fn isByRef(ty: Type) bool { return false; }, .Union => return ty.hasRuntimeBits(), - .ErrorUnion => return isByRef(ty.errorUnionPayload()), + .ErrorUnion => { + const payload_ty = ty.errorUnionPayload(); + if (!payload_ty.hasRuntimeBitsIgnoreComptime()) { + return false; + } + return true; + // TODO we actually want this logic: + // however it is tripping an LLVM 14 regression: + // https://github.com/llvm/llvm-project/issues/56585 + //return isByRef(payload_ty); + }, .Optional => { var buf: Type.Payload.ElemType = undefined; const payload_ty = ty.optionalChild(&buf); From fe8c3ffeb1c6cfb1cc0b7a81e433add3c57b2337 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 19 Jul 2022 11:31:37 -0700 Subject: [PATCH 4/4] LLVM: change commentary on isByRef This branch originally started out as a potential workaround to address #11450. It did not solve that problem, however, it did end up fixing #11498! --- src/codegen/llvm.zig | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 992ea0ff45e3..53c97c880f23 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -9356,11 +9356,11 @@ fn ccAbiPromoteInt( return null; } +/// This is the one source of truth for whether a type is passed around as an LLVM pointer, +/// or as an LLVM value. fn isByRef(ty: Type) bool { // For tuples and structs, if there are more than this many non-void // fields, then we make it byref, otherwise byval. - // TODO we actually want to set this to 2, however it is tripping an LLVM 14 regression: - // https://github.com/llvm/llvm-project/issues/56585 const max_fields_byval = 0; switch (ty.zigTypeTag()) { @@ -9421,10 +9421,6 @@ fn isByRef(ty: Type) bool { return false; } return true; - // TODO we actually want this logic: - // however it is tripping an LLVM 14 regression: - // https://github.com/llvm/llvm-project/issues/56585 - //return isByRef(payload_ty); }, .Optional => { var buf: Type.Payload.ElemType = undefined; @@ -9436,10 +9432,6 @@ fn isByRef(ty: Type) bool { return false; } return true; - // TODO we actually want this logic: - // however it is tripping an LLVM 14 regression: - // https://github.com/llvm/llvm-project/issues/56585 - //return isByRef(payload_ty); }, } }