Skip to content

Add missing transpose functions, delegate a bunch of them to gr - #2203

Merged
fredrik-johansson merged 9 commits into
flintlib:mainfrom
lgoettgens:lg/transpose
Jan 29, 2025
Merged

Add missing transpose functions, delegate a bunch of them to gr#2203
fredrik-johansson merged 9 commits into
flintlib:mainfrom
lgoettgens:lg/transpose

Conversation

@lgoettgens

Copy link
Copy Markdown
Contributor

Also make their docstrings a bit more uniform. @albinahlback this would be another candidate for #2094

@fredrik-johansson

Copy link
Copy Markdown
Collaborator

Some quick benchmark what the performance impact would be for fmpz_mat to transpose a 100x100 matrix of small values.

In-place:

fmpz_mat_transpose (old)   6.0e-6 s
gr_mat_transpose           8.6e-6 s

Not in-place:

fmpz_mat_transpose (old)   2.1e-5 s
gr_mat_transpose           3.1e-5 s

I think for other types we basically don't care about this extra overhead, but integer matrices are worth having the best possible performance.

Now if I try to optimize gr_mat_transpose with some code like this:

    if (A == B)  /* In-place, guaranteed to be square */
    {
        slong stride = A->stride;

        /* Optimize for common sizes */
        if (ctx->sizeof_elem == sizeof(ulong))
        {
            ulong * a = A->entries;

            for (i = 0; i < A->r - 1; i++)
                for (j = i + 1; j < A->c; j++)
                    FLINT_SWAP(ulong, a[i * stride + j], a[j * stride + i]);
        }
        else
        {
            gr_ptr a = A->entries;

            for (i = 0; i < A->r - 1; i++)
                for (j = i + 1; j < A->c; j++)
                    gr_swap(GR_ENTRY(a, i * stride + j, sz), GR_ENTRY(a, j * stride + i, sz), ctx);
        }
    }
    else  /* Not aliased; general case */
    {
        slong Astride = A->stride;
        slong Bstride = B->stride;
        gr_srcptr a = A->entries;
        gr_ptr b = B->entries;
        gr_method_unary_op set = GR_UNARY_OP(ctx, SET);

        for (i = 0; i < B->r; i++)
            for (j = 0; j < B->c; j++)
                status |= set(GR_ENTRY(b, i * Bstride + j, sz), GR_ENTRY(a, j * Astride + i, sz), ctx);
    }

I get

In-place          2.6e-6 s
Not in-place      2.7e-5 s

So the in-place version is suddenly extremely fast, but not in-place is still about 30% slower. Maybe we can live with that. We could recover that if we add some strided vec functions in the future.

Co-authored-by: Fredrik Johansson <fredrik.johansson@gmail.com>
@lgoettgens

Copy link
Copy Markdown
Contributor Author

I applied your patch to gr_mat_transpose and played around with it for a little bit. With benchmarking, it seems that changing the iteration order in the non-inplace variant to be cache efficient for the source matrix is more performant than for the target matrix. This reduces the regression for this case to only 8-10% on my machine.

@fredrik-johansson

Copy link
Copy Markdown
Collaborator

Cool! Positive review from me. I'll make an issue about strided vector functions also.

@fredrik-johansson
fredrik-johansson merged commit edb40b4 into flintlib:main Jan 29, 2025
@lgoettgens
lgoettgens deleted the lg/transpose branch February 4, 2025 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants