feat(convrot): select validated backend paths #44
There are no files selected for viewing
Submodule ggml
updated
43 files
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
| #include <regex> | ||
| #include <set> | ||
| #include <sstream> | ||
| #include <stdexcept> | ||
| #include <string> | ||
| #include <unordered_map> | ||
| #include <unordered_set> | ||
|
|
@@ -43,6 +44,153 @@ | |
|
|
||
| #define EPS 1e-05f | ||
|
|
||
| // Construct only the operation metadata needed for the normal backend | ||
| // supports_op query. This stays private to the loader policy: backend | ||
| // capabilities are expressed through the existing ggml interface, not a new | ||
| // public ConvRot-specific API. | ||
| inline bool ggml_backend_supports_convrot_op(ggml_backend_t backend) { | ||
| if (backend == nullptr) { | ||
| return false; | ||
| } | ||
| std::vector<uint8_t> storage(4 * ggml_tensor_overhead() + 1024); | ||
| ggml_init_params params = { | ||
| /*.mem_size =*/storage.size(), | ||
| /*.mem_buffer =*/storage.data(), | ||
| /*.no_alloc =*/true, | ||
| }; | ||
| ggml_context* ctx = ggml_init(params); | ||
| if (ctx == nullptr) { | ||
| return false; | ||
| } | ||
| ggml_tensor* activations = ggml_new_tensor_2d(ctx, GGML_TYPE_F32, 256, 1); | ||
| ggml_tensor* weights = ggml_new_tensor_2d(ctx, GGML_TYPE_I8, 256, 1); | ||
| ggml_tensor* scales = ggml_new_tensor_1d(ctx, GGML_TYPE_F32, 1); | ||
| ggml_tensor* op = ggml_mul_mat_convrot(ctx, activations, weights, scales, 256); | ||
| const bool supported = ggml_backend_supports_op(backend, op); | ||
| ggml_free(ctx); | ||
| return supported; | ||
| } | ||
|
|
||
| inline bool ggml_backend_supports_convrot_rotation_op(ggml_backend_t backend) { | ||
| if (backend == nullptr) { | ||
| return false; | ||
| } | ||
| std::vector<uint8_t> storage(3 * ggml_tensor_overhead() + 1024); | ||
| ggml_init_params params = { | ||
| /*.mem_size =*/storage.size(), | ||
| /*.mem_buffer =*/storage.data(), | ||
| /*.no_alloc =*/true, | ||
| }; | ||
| ggml_context* ctx = ggml_init(params); | ||
| if (ctx == nullptr) { | ||
| return false; | ||
| } | ||
| ggml_tensor* activations = ggml_new_tensor_2d(ctx, GGML_TYPE_F32, 256, 1); | ||
| ggml_tensor* op = ggml_convrot(ctx, activations, 256); | ||
| const bool supported = ggml_backend_supports_op(backend, op); | ||
| ggml_free(ctx); | ||
| return supported; | ||
| } | ||
|
|
||
| enum class ConvRotExecutionPath { | ||
| NATIVE, | ||
| DENSE_H256, | ||
| ROTATION_OP, | ||
| COMPAT, | ||
| }; | ||
|
|
||
| inline ConvRotExecutionPath select_convrot_execution_path(const char* backend_name, | ||
| bool rotation_op_supported, | ||
| const char* mode) { | ||
| if (mode != nullptr && std::strcmp(mode, "compat") == 0) { | ||
| return ConvRotExecutionPath::COMPAT; | ||
| } | ||
| if (mode != nullptr && std::strcmp(mode, "native") == 0) { | ||
| return ConvRotExecutionPath::NATIVE; | ||
| } | ||
| if (mode != nullptr && std::strcmp(mode, "dense") == 0) { | ||
| return ConvRotExecutionPath::DENSE_H256; | ||
| } | ||
| if (mode != nullptr && std::strcmp(mode, "op") == 0) { | ||
| return rotation_op_supported ? ConvRotExecutionPath::ROTATION_OP | ||
| : ConvRotExecutionPath::NATIVE; | ||
| } | ||
| if (mode != nullptr && std::strcmp(mode, "q8") != 0 && std::strcmp(mode, "auto") != 0) { | ||
| throw std::runtime_error("invalid SD_CONVROT_MODE; expected 'auto', 'native', 'q8', 'dense', 'op', or 'compat'"); | ||
| } | ||
|
|
||
| // The CUDA radix-4 transform is mathematically correct, but the H3 text | ||
| // encoder amplifies its rounding difference enough to change the scene. | ||
| // CUDA therefore keeps the numerically stable dense transform. Other | ||
| // backends use the standalone transform when they implement it. | ||
| if (backend_name != nullptr && std::strncmp(backend_name, "CUDA", 4) == 0) { | ||
| return ConvRotExecutionPath::DENSE_H256; | ||
| } | ||
| return rotation_op_supported ? ConvRotExecutionPath::ROTATION_OP | ||
| : ConvRotExecutionPath::NATIVE; | ||
| } | ||
|
|
||
| // Select the compact representation before any model parameter tensor is | ||
| // created. SD_CONVROT_MODE can override the automatic per-backend policy. | ||
| inline String2TensorStorage select_convrot_tensor_storage(ggml_backend_t backend, | ||
| const String2TensorStorage& source, | ||
| const std::string& component, | ||
| const std::string& prefix = "") { | ||
| // OrderedMap's default copy also copies its iterator index; rebuild it so | ||
| // this independent policy view owns a valid index into its own list. | ||
| String2TensorStorage selected; | ||
| bool has_convrot = false; | ||
| for (const auto& [name, storage] : source) { | ||
| selected.insert({name, storage}); | ||
| has_convrot = has_convrot || | ||
| (name.rfind(prefix, 0) == 0 && storage.is_comfy_int8_convrot_weight()); | ||
| } | ||
| if (!has_convrot) { | ||
| return selected; | ||
| } | ||
|
|
||
| const char* mode = std::getenv("SD_CONVROT_MODE"); | ||
| const char* backend_name = backend != nullptr ? ggml_backend_name(backend) : "unknown"; | ||
| const bool rotation_op_supported = ggml_backend_supports_convrot_rotation_op(backend); | ||
| const ConvRotExecutionPath path = select_convrot_execution_path(backend_name, rotation_op_supported, mode); | ||
| if (path == ConvRotExecutionPath::COMPAT) { | ||
| LOG_INFO("ConvRot: using explicitly selected F16 compatibility path for %s on backend %s", | ||
| component.c_str(), backend_name); | ||
| return selected; | ||
| } | ||
| if (path == ConvRotExecutionPath::DENSE_H256 || path == ConvRotExecutionPath::ROTATION_OP) { | ||
| for (auto& [name, storage] : selected) { | ||
| if (name.rfind(prefix, 0) == 0 && storage.is_comfy_int8_convrot_weight()) { | ||
| storage.comfy_int8_native_enabled = true; | ||
| storage.comfy_int8_q8_decomp_enabled = true; | ||
| storage.comfy_int8_convrot_op_enabled = path == ConvRotExecutionPath::ROTATION_OP; | ||
| } | ||
| } | ||
| LOG_INFO("ConvRot: selected Q8_0 %s activation transform for %s on backend %s", | ||
| path == ConvRotExecutionPath::ROTATION_OP ? "standalone" : "dense-H256", | ||
| component.c_str(), backend_name); | ||
| return selected; | ||
| } | ||
| if (mode != nullptr && std::strcmp(mode, "op") == 0 && !rotation_op_supported) { | ||
| LOG_WARN("ConvRot: standalone rotation is unavailable on backend %s; falling back to the native operation for %s", | ||
| backend_name, component.c_str()); | ||
| } | ||
| if (!ggml_backend_supports_convrot_op(backend)) { | ||
| throw std::runtime_error("ConvRot native support is required for " + component + | ||
| " but backend '" + backend_name + | ||
| "' lacks the 256-wide I8/F32 ConvRot operation; use a capable backend or set " | ||
| "SD_CONVROT_MODE=compat to select the F16 compatibility path"); | ||
| } | ||
| for (auto& [name, storage] : selected) { | ||
| if (name.rfind(prefix, 0) == 0 && storage.is_comfy_int8_convrot_weight()) { | ||
| storage.comfy_int8_native_enabled = true; | ||
| } | ||
| } | ||
| LOG_INFO("ConvRot: selected native compact I8/F32 path for %s on backend %s", | ||
| component.c_str(), backend_name); | ||
| return selected; | ||
| } | ||
|
|
||
| #ifndef __STATIC_INLINE__ | ||
| #define __STATIC_INLINE__ static inline | ||
| #endif | ||
|
|
@@ -1695,6 +1843,15 @@ struct WeightAdapter { | |
| ggml_tensor* b, | ||
| const std::string& prefix, | ||
| ForwardParams forward_params) = 0; | ||
| // Return only the adapter's output-space contribution. Native operations | ||
| // such as compact ConvRot own their base-weight arithmetic and therefore | ||
| // cannot use forward_with_lora() without recomputing an incompatible base. | ||
| virtual ggml_tensor* lora_output_delta(ggml_context* ctx, | ||
| ggml_backend_t backend, | ||
| ggml_tensor* x, | ||
| ggml_tensor* w, | ||
| const std::string& prefix, | ||
| ForwardParams forward_params) = 0; | ||
| virtual size_t get_extra_graph_size() = 0; | ||
| }; | ||
|
|
||
|
|
@@ -3973,12 +4130,51 @@ class Linear : public UnaryBlock { | |
| bool force_prec_f32; | ||
| bool allow_weight_scale; | ||
| bool has_weight_scale = false; | ||
| // This is distinct from `weight_scale`: the latter is a regular | ||
| // post-linear model parameter, while ConvRot's F32 vector is a private | ||
| // sidecar input to GGML_OP_MUL_MAT_CONVROT. | ||
| bool has_convrot_weight = false; | ||
| bool use_convrot_f16_compat = false; | ||
| bool use_convrot_q8_decomp = false; | ||
| bool use_convrot_rotation_op = false; | ||
| bool use_convrot_fast_h256 = false; | ||
| float scale; | ||
| std::string prefix; | ||
|
|
||
| void init_params(ggml_context* ctx, const String2TensorStorage& tensor_storage_map = {}, const std::string prefix = "") override { | ||
| this->prefix = prefix; | ||
| has_weight_scale = false; | ||
| has_convrot_weight = false; | ||
| use_convrot_f16_compat = false; | ||
| use_convrot_q8_decomp = false; | ||
| use_convrot_rotation_op = false; | ||
| use_convrot_fast_h256 = false; | ||
| const auto storage_it = tensor_storage_map.find(prefix + "weight"); | ||
| if (storage_it != tensor_storage_map.end() && storage_it->second.is_comfy_int8_convrot_weight() && !force_f32 && | ||
| storage_it->second.comfy_int8_native_enabled) { | ||
| has_convrot_weight = true; | ||
| if (storage_it->second.comfy_int8_q8_decomp_enabled) { | ||
| params["weight"] = ggml_new_tensor_2d(ctx, GGML_TYPE_Q8_0, in_features, out_features); | ||
| use_convrot_q8_decomp = true; | ||
| use_convrot_rotation_op = storage_it->second.comfy_int8_convrot_op_enabled; | ||
| if (!use_convrot_rotation_op) { | ||
| params["weight.convrot_h256"] = ggml_new_tensor_2d(ctx, GGML_TYPE_F32, 256, 256); | ||
| const char* h256_mode = std::getenv("SD_CONVROT_H256_MODE"); | ||
| use_convrot_fast_h256 = h256_mode != nullptr && std::strcmp(h256_mode, "fast") == 0; | ||
| if (h256_mode != nullptr && !use_convrot_fast_h256 && std::strcmp(h256_mode, "dense") != 0) { | ||
| throw std::runtime_error("invalid SD_CONVROT_H256_MODE; expected 'dense' or 'fast'"); | ||
| } | ||
| } | ||
| } else { | ||
| params["weight"] = ggml_new_tensor_2d(ctx, GGML_TYPE_I8, in_features, out_features); | ||
| params["weight.convrot_scale"] = ggml_new_tensor_1d(ctx, GGML_TYPE_F32, out_features); | ||
| use_convrot_f16_compat = storage_it->second.name.rfind("text_encoders.llm.", 0) == 0; | ||
| } | ||
| if (bias) { | ||
| params["bias"] = ggml_new_tensor_1d(ctx, GGML_TYPE_F32, out_features); | ||
| } | ||
| return; | ||
| } | ||
| enum ggml_type wtype = get_type(prefix + "weight", tensor_storage_map, GGML_TYPE_F32); | ||
| if (in_features % ggml_blck_size(wtype) != 0 || force_f32) { | ||
| wtype = GGML_TYPE_F32; | ||
|
|
@@ -4026,7 +4222,61 @@ class Linear : public UnaryBlock { | |
| } | ||
| ggml_tensor* linear_bias = has_weight_scale ? nullptr : b; | ||
| ggml_tensor* out = nullptr; | ||
| if (has_convrot_weight) { | ||
| if (use_convrot_q8_decomp) { | ||
| ggml_tensor* scaled_x = scale != 1.f ? ggml_ext_scale(ctx->ggml_ctx, x, scale) : x; | ||
| if (use_convrot_rotation_op) { | ||
| ggml_tensor* rotated = ggml_convrot(ctx->ggml_ctx, scaled_x, 256); | ||
| ggml_set_name(rotated, (prefix + "trace.convrot.post_h256").c_str()); | ||
| out = ggml_mul_mat(ctx->ggml_ctx, w, rotated); | ||
| } else { | ||
| // H256 is symmetric: (H*w_row).x == w_row.(H*x). Rotate | ||
| // each 256-wide block before the stock Q8_0 matmul. | ||
| ggml_tensor* contiguous = ggml_is_contiguous(scaled_x) ? scaled_x : ggml_cont(ctx->ggml_ctx, scaled_x); | ||
| ggml_tensor* blocks = ggml_reshape_2d(ctx->ggml_ctx, contiguous, 256, | ||
| ggml_nelements(contiguous) / 256); | ||
| ggml_set_name(blocks, (prefix + "trace.convrot.pre_h256").c_str()); | ||
| ggml_tensor* rotated = ggml_mul_mat(ctx->ggml_ctx, params["weight.convrot_h256"], blocks); | ||
| ggml_set_name(rotated, (prefix + "trace.convrot.post_h256").c_str()); | ||
| if (use_convrot_fast_h256) { | ||
| ggml_mul_mat_set_hint(rotated, GGML_HINT_SRC0_IS_CONVROT_H256); | ||
| } | ||
| rotated = ggml_reshape_4d(ctx->ggml_ctx, rotated, x->ne[0], x->ne[1], x->ne[2], x->ne[3]); | ||
| out = ggml_mul_mat(ctx->ggml_ctx, w, rotated); | ||
| } | ||
| if (force_prec_f32) { | ||
| ggml_mul_mat_set_prec(out, GGML_PREC_F32); | ||
| } | ||
| ggml_set_name(out, (prefix + "trace.convrot.post_q8_gemm").c_str()); | ||
| if (scale != 1.f) { | ||
| out = ggml_ext_scale(ctx->ggml_ctx, out, 1.f / scale); | ||
| } | ||
| } else { | ||
| // ConvRot weights and their tensor-wise scales remain compact | ||
| // at rest. The operator owns the scale semantics. | ||
| out = ggml_mul_mat_convrot(ctx->ggml_ctx, x, w, params["weight.convrot_scale"], 256); | ||
| ggml_set_name(out, (prefix + "trace.convrot.native_out").c_str()); | ||
| if (use_convrot_f16_compat) { | ||
| ggml_mul_mat_convrot_set_f16_compat(out, true); | ||
| } | ||
| } | ||
| if (b != nullptr) { | ||
| if (ctx->weight_adapter) { | ||
| b = ctx->weight_adapter->patch_weight(ctx->ggml_ctx, ctx->backend, b, prefix + "bias"); | ||
| } | ||
| out = ggml_add_inplace(ctx->ggml_ctx, out, b); | ||
| } | ||
| if (ctx->weight_adapter) { | ||
| WeightAdapter::ForwardParams forward_params; | ||
| forward_params.op_type = WeightAdapter::ForwardParams::op_type_t::OP_LINEAR; | ||
| forward_params.linear.force_prec_f32 = force_prec_f32; | ||
| forward_params.linear.scale = scale; | ||
| if (ggml_tensor* delta = ctx->weight_adapter->lora_output_delta( | ||
| ctx->ggml_ctx, ctx->backend, x, w, prefix, forward_params)) { | ||
| out = ggml_add_inplace(ctx->ggml_ctx, out, delta); | ||
| } | ||
| } | ||
|
Comment on lines
+4269
to
+4278
There was a problem hiding this comment. Under the default AUTO switches to runtime LoRA only when
Impact: any LoRA on a ConvRot model under default settings silently corrupts the affected layers and the generation; the runtime Suggested fix: count ConvRot storage as quantized in the AUTO decision, so these models take the runtime path for (const auto& [name, ts] : model_loader.get_tensor_storage_map()) {
if (ts.is_comfy_int8_tensorwise) {
have_quantized_weight = true;
break;
}
}For an explicit On the runtime path, LoHa and raw
Author
There was a problem hiding this comment. Addressed in |
||
| } else if (ctx->weight_adapter) { | ||
| WeightAdapter::ForwardParams forward_params; | ||
| forward_params.op_type = WeightAdapter::ForwardParams::op_type_t::OP_LINEAR; | ||
| forward_params.linear.force_prec_f32 = force_prec_f32; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1305,6 +1305,28 @@ struct MultiLoraAdapter : public WeightAdapter { | |
| return out; | ||
| } | ||
|
|
||
| ggml_tensor* lora_output_delta(ggml_context* ctx, | ||
| ggml_backend_t backend, | ||
| ggml_tensor* x, | ||
| ggml_tensor* w, | ||
| const std::string& prefix, | ||
| WeightAdapter::ForwardParams forward_params) override { | ||
| ggml_tensor* delta = nullptr; | ||
| for (auto& lora_model : lora_models) { | ||
| if (ggml_tensor* diff = lora_model->get_weight_diff(prefix + "weight", backend, ctx, w, false)) { | ||
| ggml_tensor* current = ggml_ext_linear(ctx, x, diff, nullptr, | ||
| forward_params.linear.force_prec_f32, | ||
| forward_params.linear.scale); | ||
| delta = delta == nullptr ? current : ggml_add_inplace(ctx, delta, current); | ||
| } | ||
| ggml_tensor* current = lora_model->get_out_diff(ctx, backend, x, w, forward_params, prefix + "weight"); | ||
| if (current != nullptr) { | ||
| delta = delta == nullptr ? current : ggml_add_inplace(ctx, delta, current); | ||
| } | ||
| } | ||
| return delta; | ||
|
Comment on lines
+1308
to
+1327
There was a problem hiding this comment.
Impact: LoHa adapters, full-diff patches and bias diffs are silent no-ops on every ConvRot layer while the run reports success. Once AUTO routes ConvRot models to runtime LoRA, this is the only LoRA path they have. Suggested fix: apply the weight-diff kinds in output space, inside the existing per-model loop, before the if (ggml_tensor* diff = lora_model->get_weight_diff(prefix + "weight", backend, ctx, w, false)) {
ggml_tensor* current = ggml_mul_mat(ctx, diff, x);
delta = delta == nullptr ? current : ggml_add_inplace(ctx, delta, current);
}
if (ctx->weight_adapter) {
b = ctx->weight_adapter->patch_weight(ctx->ggml_ctx, ctx->backend, b, prefix + "bias");
}
Author
There was a problem hiding this comment. Addressed in |
||
| } | ||
|
|
||
| size_t get_extra_graph_size() override { | ||
| size_t lora_tensor_num = 0; | ||
| for (auto& lora_model : lora_models) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Q8_0 ConvRot matmuls bypass
ggml_ext_linear, so MiniMax-H3'sfc1/fc2lose theirforce_prec_f32=true/scale=1/128F16-overflow guard on the new default paths.minimax_h3.hpp:158-159declaresfc1/fc2withforce_prec_f32 = trueandscale = 1/128.ggml_ext_linear(ggml_extend.hpp:1158-1190) honours both: it prescalesx, setsGGML_PREC_F32on the matmul, and undoes the scale. The Q8_0 ConvRot branch callsggml_mul_mat(w, rotated)directly (:4230,:4244) and does neither. On Vulkan, a Q8_0 matmul atGGML_PREC_DEFAULTselects thef16accpipeline on fp16 devices (ggml-vulkan.cpp:8926-8935), so the H3 FFN now accumulates in F16 without the guard the model code added for it.Impact: latent inf/NaN or precision loss in the H3 FFN on the Vulkan default path. The matched Strix run was correct, so this depends on the workload rather than failing today.
Suggested fix: in the
use_convrot_q8_decompbranch, scale a copy ofx— keepxitself unscaled, sincelora_output_deltabelow consumes it — and restore after the final matmul, before the bias add:The rotation is linear, so prescaling before it is exact.
ggml_mul_mat_set_precassertsGGML_OP_MUL_MAT, so this applies to the two Q8_0 paths only; leave the nativeggml_mul_mat_convrotpath as is. On Vulkan this selectsf32acc, so re-run the Vulkan benchmark.Separately,
force_f32Linears (proj_in/proj_out,*_patch_proj,video_out/audio_out) skipforce_f32when the file marks them ConvRot. Adding&& !force_f32to the ConvRot condition ininit_paramsroutes them to the existing F32 reconstruction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in
f8fa0ee. Both Q8_0 paths now prescale activations atsrc/core/ggml_extend.hpp:4227, request F32 matmul accumulation atsrc/core/ggml_extend.hpp:4247, and restore scale atsrc/core/ggml_extend.hpp:4251. ConvRot Linears marked force_f32 use F32 reconstruction atsrc/core/ggml_extend.hpp:4153. Graph checks cover the rotation-op and dense paths; they passed on NV5090, Strix, and Mac Studio. The full Vulkan performance benchmark has not been rerun.