From f8def7fe168bab245fbf15d3f18b26dbb1ef73c8 Mon Sep 17 00:00:00 2001 From: Yash Raj Pandey <55940078+devYRPauli@users.noreply.github.com> Date: Mon, 10 Aug 2026 08:01:44 -0400 Subject: [PATCH] ggml : require contiguous src for ROLL on CUDA and Metal (#25928) ggml_roll only asserts nb[0] == ggml_type_size, so a permuted src is a valid input, but the CUDA and Metal roll kernels index by ne alone and never read the nb strides. A non-contiguous src therefore produced silently wrong results. Neither backend declared a contiguity requirement in supports_op, so the scheduler did not fall back to the CPU implementation, which does handle strides correctly. Add the requirement to both backends, matching the existing GGML_OP_ROPE guard, and add a permuted test_roll case. --- ggml/src/ggml-cuda/ggml-cuda.cu | 2 +- ggml/src/ggml-metal/ggml-metal-device.m | 3 ++- tests/test-backend-ops.cpp | 14 +++++++++++--- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/ggml/src/ggml-cuda/ggml-cuda.cu b/ggml/src/ggml-cuda/ggml-cuda.cu index dec619324..05e8d7f73 100644 --- a/ggml/src/ggml-cuda/ggml-cuda.cu +++ b/ggml/src/ggml-cuda/ggml-cuda.cu @@ -5185,7 +5185,7 @@ static bool ggml_backend_cuda_device_supports_op(ggml_backend_dev_t dev, const g return max_bias == 0.0f; } case GGML_OP_ROLL: - if(op->src[0]->type == GGML_TYPE_F32) { + if(op->src[0]->type == GGML_TYPE_F32 && ggml_is_contiguous(op->src[0])) { return true; } return false; diff --git a/ggml/src/ggml-metal/ggml-metal-device.m b/ggml/src/ggml-metal/ggml-metal-device.m index 2dc6eb8fd..df85ab02c 100644 --- a/ggml/src/ggml-metal/ggml-metal-device.m +++ b/ggml/src/ggml-metal/ggml-metal-device.m @@ -1268,8 +1268,9 @@ bool ggml_metal_device_supports_op(ggml_metal_device_t dev, const struct ggml_te case GGML_OP_ARGSORT: case GGML_OP_TOP_K: case GGML_OP_ARANGE: - case GGML_OP_ROLL: return true; + case GGML_OP_ROLL: + return ggml_is_contiguous(op->src[0]); case GGML_OP_FLASH_ATTN_EXT: // for new head sizes, add checks here if (op->src[0]->ne[0] != 32 && diff --git a/tests/test-backend-ops.cpp b/tests/test-backend-ops.cpp index 14a234060..a4c22156c 100644 --- a/tests/test-backend-ops.cpp +++ b/tests/test-backend-ops.cpp @@ -6712,19 +6712,26 @@ struct test_roll : public test_case { const int shift1; const int shift3; const int shift4; + const bool permute; std::string vars() override { - return VARS_TO_STR4(shift0, shift1, shift3, shift4); + return VARS_TO_STR5(shift0, shift1, shift3, shift4, permute); } - test_roll(int shift0 = 3, int shift1 = -2, int shift3 = 1, int shift4 = -1) - : shift0(shift0), shift1(shift1), shift3(shift3), shift4(shift4) {} + test_roll(int shift0 = 3, int shift1 = -2, int shift3 = 1, int shift4 = -1, bool permute = false) + : shift0(shift0), shift1(shift1), shift3(shift3), shift4(shift4), permute(permute) {} ggml_tensor * build_graph(ggml_context * ctx) override { int64_t ne[4] = {10, 5, 4, 3}; ggml_tensor * a = ggml_new_tensor(ctx, GGML_TYPE_F32, 4, ne); ggml_set_name(a, "a"); + if (permute) { + // ggml_roll only requires nb[0] == type size, so a permuted src is valid + a = ggml_permute(ctx, a, 0, 2, 1, 3); + ggml_set_name(a, "a_permuted"); + } + ggml_tensor * out = ggml_roll(ctx, a, shift0, shift1, shift3, shift4); ggml_set_name(out, "out"); @@ -9459,6 +9466,7 @@ static std::vector> make_test_cases_eval() { test_cases.emplace_back(new test_pad_reflect_1d()); test_cases.emplace_back(new test_pad_reflect_1d(GGML_TYPE_F32, {3000, 384, 4, 1})); test_cases.emplace_back(new test_roll()); + test_cases.emplace_back(new test_roll(3, -2, 1, -1, true)); test_cases.emplace_back(new test_arange()); test_cases.emplace_back(new test_arange(GGML_TYPE_F32, 0.0f, 1048576.0f, 1.0f)); test_cases.emplace_back(new test_timestep_embedding());