Skip to content

design flaw: SIMD vectors are indexable in an inconsistent way #5427

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

Closed
andrewrk opened this issue May 25, 2020 · 7 comments
Closed

design flaw: SIMD vectors are indexable in an inconsistent way #5427

andrewrk opened this issue May 25, 2020 · 7 comments
Milestone

Comments

@andrewrk
Copy link
Member

SIMD: #903

Thanks @daurnimator for pointing this out in #5425 (comment).

Currently, vectors are indexable:

test "load vector elements via comptime index" {
const S = struct {
fn doTheTest() void {
var v: Vector(4, i32) = [_]i32{ 1, 2, 3, undefined };
expect(v[0] == 1);
expect(v[1] == 2);
expect(loadv(&v[2]) == 3);
}
fn loadv(ptr: var) i32 {
return ptr.*;
}
};
S.doTheTest();
comptime S.doTheTest();
}
test "store vector elements via comptime index" {
const S = struct {
fn doTheTest() void {
var v: Vector(4, i32) = [_]i32{ 1, 5, 3, undefined };
v[2] = 42;
expect(v[1] == 5);
v[3] = -364;
expect(v[2] == 42);
expect(-364 == v[3]);
storev(&v[0], 100);
expect(v[0] == 100);
}
fn storev(ptr: var, x: i32) void {
ptr.* = x;
}
};
S.doTheTest();
comptime S.doTheTest();
}
test "load vector elements via runtime index" {
const S = struct {
fn doTheTest() void {
var v: Vector(4, i32) = [_]i32{ 1, 2, 3, undefined };
var i: u32 = 0;
expect(v[i] == 1);
i += 1;
expect(v[i] == 2);
i += 1;
expect(v[i] == 3);
}
};
S.doTheTest();
comptime S.doTheTest();
}
test "store vector elements via runtime index" {
const S = struct {
fn doTheTest() void {
var v: Vector(4, i32) = [_]i32{ 1, 5, 3, undefined };
var i: u32 = 2;
v[i] = 1;
expect(v[1] == 5);
expect(v[2] == 1);
i += 1;
v[i] = -364;
expect(-364 == v[3]);
}
};
S.doTheTest();
comptime S.doTheTest();
}

Using an index accesses the vector elements. However, this conflicts with the plan to have vectors of single-item pointers to structs. Already it is strange, because if you can index something then you would expect the len property to work. But vectors do not support this because if there is a vector of single-item pointers to structs, then one of the fields could be len.

But vectors could also be vectors of single-item pointers to arrays! In this case, in the same way that field access would work for the vector of struct pointers, indexing the vector should work on the inner array pointers.

Related: #5425 this would need to get reworked to check the element type

@andrewrk andrewrk added this to the 0.7.0 milestone May 25, 2020
@data-man
Copy link
Contributor

@andrewrk

Related: #5425 this would need to get reworked to check the element type

What's wrong?

const std = @import("std");
const meta = std.meta;
const warn = std.debug.warn;
const isIndexable = meta.trait.isIndexable;

pub fn main() void {
    const int32: i32 = 0;
    const array = [_]u8{0} ** 10;
    const vector: meta.Vector(2, u32) = [_]u32{0} ** 2;

    warn("isIndexable(int32): {}\n", .{isIndexable(@TypeOf(int32))});
    warn("isIndexable(&int32): {}\n", .{isIndexable(@TypeOf(&int32))});
    warn("isIndexable(array): {}\n", .{isIndexable(@TypeOf(array))});
    warn("isIndexable(&array): {}\n", .{isIndexable(@TypeOf(&array))});
    warn("isIndexable(vector): {}\n", .{isIndexable(@TypeOf(vector))});
    warn("isIndexable(&vector): {}\n", .{isIndexable(@TypeOf(&vector))});

    warn("@typeInfo(&vector): {}\n", .{@typeInfo(@TypeOf(&vector))});
}

Outputs:

isIndexable(int32): false
isIndexable(&int32): false
isIndexable(array): true
isIndexable(&array): true
isIndexable(vector): true
isIndexable(&vector): false
@typeInfo(&vector): TypeInfo{ .Pointer = Pointer{ .size = Size.One, .is_const = true, .is_volatile = false, .alignment = 8, .child = type, .is_allowzero = false, .sentinel = null } }

@andrewrk
Copy link
Member Author

@data-man with the way the language works right now, there is nothing wrong with #5425, it is correct. However the language itself has an inconsistency in it

@shawnl
Copy link
Contributor

shawnl commented Jun 6, 2020

This same discussion has happened over and over again for months, and I have answered it a number of times: if the index is scalar it addresses the element numbers. The extension is that if it is a vector, then it does a @shuffle(). All pointer arithmetic (everything that can be done with [] on a scalar pointer) can be done with + and - instead of [].

Both gcc and clang work this way.

Unrelated, the big thing that caused me to drop my patch series on the floor was a technical mistake that means zig can't match gcc's feature set: runtime vector indexes. (that doesn't mean I understand how to use ResultLoc, which was the reason I couldn't get my code working doing it the way gcc does it).

    // If the instruction is a element pointer instruction to a vector, we emit
    // vector element extract instruction rather than load pointer. If the
    // pointer type has non-VECTOR_INDEX_RUNTIME value, it would have been
    // possible to implement this in the codegen for IrInstGenLoadPtr.
    // However if it has VECTOR_INDEX_RUNTIME then we must emit a compile error
    // if the vector index cannot be determined right here, right now, because
    // the type information does not contain enough information to actually
    // perform a dereference.
    if (ptr_type->data.pointer.vector_index == VECTOR_INDEX_RUNTIME) {
        if (ptr->id == IrInstGenIdElemPtr) {
            IrInstGenElemPtr *elem_ptr = (IrInstGenElemPtr *)ptr;
            IrInstGen *vector_loaded = ir_get_deref(ira, &elem_ptr->array_ptr->base,
                    elem_ptr->array_ptr, nullptr);
            IrInstGen *elem_index = elem_ptr->elem_index;
            return ir_build_vector_extract_elem(ira, source_instruction, vector_loaded, elem_index);
        }
        ir_add_error(ira, &ptr->base,
            buf_sprintf("unable to determine vector element index of type '%s'", buf_ptr(&ptr_type->name)));
        return ira->codegen->invalid_inst_gen;
    }

isIndexable(&vector): false

@data-man This can be fixed, and gcc supports this. I dropped all my work on the floor when it looked like @andrewrk couldn't change his mind about an incorrect technical decision. (which was made while I was trying to tell him it was incorrect)

@andrewrk
Copy link
Member Author

andrewrk commented Jun 9, 2020

This same discussion has happened over and over again for months, and I have answered it a number of times: if the index is scalar it addresses the element numbers.

You are absolutely correct. I apologize for forgetting this again.

@andrewrk couldn't change his mind about an incorrect technical decision. (which was made while I was trying to tell him it was incorrect)

I don't know what you are referring to. Please just open an issue to explain the situation rather than saying negative things about me

@shawnl
Copy link
Contributor

shawnl commented Jun 10, 2020

I am sorry if that came across as an ad homonim.

@daurnimator
Copy link
Contributor

I'm confused about why this was closed; the inconsistency still exists.

@andrewrk
Copy link
Member Author

As @shawnl noted, if the index is scalar then it indexes elements. If the index is a vector, then it can perform array element access or struct field access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants