Skip to content

Remove array type coercion and fix result location bugs #3787

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

Merged
merged 21 commits into from
Dec 2, 2019

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Nov 27, 2019

  • Implements remove type coercion from array values to references #3768. This is a sweeping breaking change that requires
    many (trivial) edits to Zig source code. Array values no longer
    coerce to slices; however one may use & to obtain a reference to
    an array value, which may then be coerced to a slice.

  • Adds IrInstruction::dump, for debugging purposes. It's useful to
    call to inspect the instruction when debugging Zig IR.

  • Fixes bugs with result location semantics. See the new behavior test
    cases, and compile error test cases.

  • Fixes bugs with @typeInfo not properly resolving const values.

Total bytes used by the compiler (as measured by memory_profiling.cpp) to generate std lib test binary went down by 257 MiB in this branch:

  • master: 3.198 GiB
  • this branch: 2.947 GiB

Vexu and others added 3 commits November 26, 2019 14:32
 * Implements #3768. This is a sweeping breaking change that requires
   many (trivial) edits to Zig source code. Array values no longer
   coerced to slices; however one may use `&` to obtain a reference to
   an array value, which may then be coerced to a slice.

 * Adds `IrInstruction::dump`, for debugging purposes. It's useful to
   call to inspect the instruction when debugging Zig IR.

 * Fixes bugs with result location semantics. See the new behavior test
   cases, and compile error test cases.

 * Fixes bugs with `@typeInfo` not properly resolving const values.

 * Behavior tests are passing but std lib tests are not yet. There
   is more work to do before merging this branch.
@andrewrk andrewrk added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Nov 27, 2019
Having ConstGlobalRefs be a pointer in ZigValue was a hack that caused
plenty of bugs. It was used to work around difficulties in type coercing
array values into slices.

However, after #3787 is merged, array values no longer type coerce into
slices, and so this provided an opportunity to clean up the code.

This has the nice effect of reducing stage1 peak RAM usage during the
std lib tests from 3.443 GiB to 3.405 GiB (saving 39 MiB).

There is one behavior test failing in this branch, which I plan to debug
after merging #3787.
@mikdusan
Copy link
Member

mikdusan commented Dec 1, 2019

const std = @import("std");
                
const S0 = struct {
    bar: S1,    
            
    pub const S1 = struct {
        value: u8,  
    };              
                    
    fn init() @This() {
        return S0{ .bar = S1{ .value = 123 } };
    }           
};          
                
var g_foo: S0 = S0.init();

pub fn main() void {
    std.debug.warn("{*}\n", &g_foo);
    g_foo.bar.value = 42;
}

BASELINE

@g_foo = internal unnamed_addr global %S0 { %S1 { i8 123 } }, align 1
...
define internal fastcc void @main.0() unnamed_addr #1 !dbg !13095 {
Entry:
  call fastcc void @std.debug.warn.121(%S0* @g_foo), !dbg !13097
  store i8 42, i8* getelementptr inbounds (%S0, %S0* @g_foo, i32 0, i32 0, i32 0), align 1, !dbg !13099
  ret void, !dbg !13100
}

PR

@g_foo = internal unnamed_addr global %S0 { %S1 { i8 123 } }, align 1
@265 = internal unnamed_addr constant %S0 { %S1 { i8 123 } }, align 1
...
define internal fastcc void @main.0() unnamed_addr #1 !dbg !13095 {
Entry:
  call fastcc void @std.debug.warn.121(%S0* @g_foo), !dbg !13097
  store i8 42, i8* getelementptr inbounds (%S0, %S0* @265, i32 0, i32 0, i32 0), align 1, !dbg !13099
  ret void, !dbg !13100
}

andrewrk added a commit that referenced this pull request Dec 1, 2019
Having ConstGlobalRefs be a pointer in ZigValue was a hack that caused
plenty of bugs. It was used to work around difficulties in type coercing
array values into slices.

However, after #3787 is merged, array values no longer type coerce into
slices, and so this provided an opportunity to clean up the code.

This has the nice effect of reducing stage1 peak RAM usage during the
std lib tests from 3.443 GiB to 3.405 GiB (saving 39 MiB).

There is one behavior test failing in this branch, which I plan to debug
after merging #3787.
@andrewrk
Copy link
Member Author

andrewrk commented Dec 2, 2019

This branch is big enough, I'm giving up on solving these test cases in the branch, will attempt again in a new branch after this branch is merged.

    cases.add(
        "function call assigned to incorrect type",
        \\export fn entry() void {
        \\    var arr: [4]f32 = undefined;
        \\    arr = concat();
        \\}
        \\fn concat() [16]f32 {
        \\    return [1]f32{0}**16;
        \\}
    ,
        "tmp.zig:3:17: error: expected type '[4]f32', found '[16]f32'"
    );

    cases.add(
        "generic function call assigned to incorrect type",
        \\pub export fn entry() void {
        \\    var res: []i32 = undefined;
        \\    res = myAlloc(i32);
        \\}
        \\fn myAlloc(comptime arg: type) anyerror!arg{
        \\    unreachable;
        \\}
    ,
        "tmp.zig:3:18: error: expected type '[]i32', found 'anyerror!i32"
    );

@andrewrk andrewrk marked this pull request as ready for review December 2, 2019 01:55
@andrewrk andrewrk merged commit fecd540 into master Dec 2, 2019
@andrewrk andrewrk deleted the remove-array-type-coercion branch December 2, 2019 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants