Binary cache sql backend

[pnikolic-amd]

Motivation

Create alternative to file based binary cache. Possible benefits of sql cache are: writing/reding from one file, faster write/read, saving additional data that relates to kernel usage.

Technical Details

• Extended migraphx::sqlite with prepared statements. The existing wrapper was sqlite3_exec-based, text-only, and had
  no parameter binding, so it could not carry BLOBs. Added sqlite_stmt (bind/step/reset/column), an RAII
  sqlite_stmt_reset guard, plus prepare, set_busy_timeout, and a non-throwing try_write. Strictly additive — none of
  the three existing consumers changed, and sqlite3.h stayed out of the GPU target.
• Extracted a storage backend abstraction, with no behavior change. Generated a type-erased binary_cache_backend
  concept from a te.py DSL input, copying the problem_cache_backend precedent. Hoisted binary_cache::entry into its
  own header so the interface could name it, moved the existing filesystem code into file_binary_cache unchanged, and
  left the shared msgpack decode and key-collision check above the storage layer so both backends inherit them.
  binary_cache now holds an optional<binary_cache_backend>. The only visible change was two warning messages naming
  the key hash instead of a file path.
• Added sqlite_binary_cache. One cache_v1 table keyed (version, device, key_hash), holding the msgpack entry as a BLOB
  byte-identical to a .mxr body, with op_name/problem/solution denormalized into columns for SQL inspection, plus a
  cache_info_v1 provenance table. Statements are prepared once and reused, INSERT OR REPLACE stands in for
  publish-by-rename, and busy_timeout(5000) is the whole cross-process strategy. Every failure degrades to a recompile
  — an unopenable database disables persistence, a failed store statement leaves the cache read-only.
• Selected the backend by file extension. Added one file-static factory mirroring make_problem_cache_backend, with
  zero new configuration surface. Kept the version_dir()-empty guard in the binary_cache constructor rather than
  folding it into the factory, and made sqlite_binary_cache::open create the parent directory so a .db path behaves
  like a directory path on a fresh machine. target.cpp and binary_cache_settings were left untouched.

[Copilot]

🟡 Changes recommended

The cache initialization and SQLite failure-handling issues must be resolved before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds a SQLite-backed GPU binary cache while retaining filesystem storage.

Changes:

• Extended SQLite support with prepared statements and BLOB handling.
• Added type-erased file and SQLite cache backends selected by extension.
• Added backend and SQLite tests.

File summaries

File	Description	Review findings
tools/include/gpu/binary_cache_backend.hpp	Defines the backend type-erasure template.	None.
test/sqlite.cpp	Tests prepared statements and BLOB handling.	None.
test/gpu/binary_cache.cpp	Tests cache backends and selection.	Nit: Preserve integration coverage for directory-backed caching.
src/targets/gpu/sqlite_binary_cache.cpp	Implements SQLite persistence.	Moderate: Add a read-only fallback and disable writes after the first failure. Nit: Verify denormalized column values.
src/targets/gpu/include/migraphx/gpu/sqlite_binary_cache.hpp	Declares SQLite storage.	Nit: Clarify partial-backend behavior after statement-preparation failures.
src/targets/gpu/include/migraphx/gpu/file_binary_cache.hpp	Declares filesystem storage.	None.
src/targets/gpu/include/migraphx/gpu/binary_cache.hpp	Integrates optional storage backends.	None.
src/targets/gpu/include/migraphx/gpu/binary_cache_entry.hpp	Extracts the serialized cache entry.	None.
src/targets/gpu/include/migraphx/gpu/binary_cache_backend.hpp	Adds generated backend dispatch.	None.
src/targets/gpu/file_binary_cache.cpp	Implements filesystem persistence.	None.
src/targets/gpu/CMakeLists.txt	Builds the new backend sources.	None.
src/targets/gpu/binary_cache.cpp	Selects and operates the storage backend.	Moderate: Avoid computing version_dir() when persistence is disabled. Nit: Document extension-based selection and database maintenance.
src/sqlite.cpp	Implements statement binding, stepping, and BLOB access.	None.
src/include/migraphx/sqlite.hpp	Exposes prepared-statement APIs.	None.

Review details

Suppressed comments (2)

src/targets/gpu/include/migraphx/gpu/sqlite_binary_cache.hpp:52

• The documented contract says any statement-preparation failure returns nullopt, but open() catches store/info preparation failures and still returns a partially usable backend. Describe which failures disable the backend and which leave available operations usable.

    /// Open the database, create the schema and prepare the statements. `stamp` is the text
    /// recorded in cache_info_v1 to describe the build. Returns nullopt when any of that fails,
    /// so an unusable database leaves the cache memory-only rather than raising an error.
    /// Returns the wrapper so the caller can hand the result straight back.

src/targets/gpu/sqlite_binary_cache.cpp:199

• No test verifies the new denormalized op_name, problem, and solution column values; current database tests only count rows or reload the authoritative blob. A swapped binding or incorrect JSON representation would therefore pass. Add a query that asserts all three stored values against the entry.

            .bind(4, e.op_name)
            .bind(5, to_json_string(e.problem))
            .bind(6, to_json_string(e.solution))
            .bind(7, blob);

• Files reviewed: 14/14 changed files
• Comments generated: 5
• Review effort level: Balanced

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[pfultz2]

I am not sure about this though.

[pfultz2]

All these methods should be private. A call operator should be execute the statement and return a range over the rows. Each of the columns should be converted to the value class. Something like this:

    struct rows
    {
        rows(sqlite_stmt s) : stmt(s)
        {}

        struct iterator : iterator_operators<iterator>
        {
            using value_type        = value;
            using reference         = value_type;
            using difference_type   = std::ptrdiff_t;
            using iterator_category = std::input_iterator_tag;
            using pointer           = std::add_pointer_t<std::remove_reference_t<reference>>;
            
            iterator() = default;

            iterator(const rows* pparent)
                : parent(pparent), available(pparent->stmt.step())
            {
            }

            iterator(const rows* pparent, bool a)
                : parent(pparent), available(a)
            {
            }

            reference operator*() const
            { 
                // Add a method to conver the columns to a `value` type
                return parent->stmt.to_value();
            }

            template <class U>
            static void increment(U& x)
            {
                x.available = x.parent->stmt.step();
            }

            template <class U, class V>
            static auto equal(const U& x, const V& y)
            {
                return x.parent == y.parent and x.available == y.available;
            }

        private:
            rows* parent = nullptr;
            bool available = false;
        };

        iterator begin() { return iterator{this}; }
        iterator end() { return iterator{this, false}; }

    private:
        sqlite_stmt smt;
    };
    
    template<class... Ts>
    auto operator()(const Ts&... xs) const
    {
        // Clear any bindings first
        reset();
        // Call bind on each argument passed in
        sequence_c<sizeof...(Ts)>([&](auto... is) {
            swallow{(bind(is+1, xs), 0)...};
        });

        return rows(*this);
    }

So then we can read the rows using:

for(auto row:get_stmt(version, device, key_hash))
    return row["kernel"].get_binary(); // or whatever the column name is for where the kernel is stored
return nullopt;

[pfultz2]

I dont think we need this. We should always call reset before binding and executing.

[pfultz2]

I am removing the stamp as its clunky to past. Instead there will be a version_id function.

[pfultz2]

Does this still test the directory-based version on linux?

[pfultz2]

This should return a binary_cache_entry.

[pfultz2]

The blob parameter should be removed. It just duplicates the binary_cache_entry.

I think the backends should decide how they want to serialize the entry. sqlite could make each field a seperate column whereas the file-based would just serialize it to msgpack.

[pfultz2]

Remove this. Batches should be written by passing a vector into store. There is also no reason to add another overload for this either.

[pfultz2]

This should take a vector of entries.

[pfultz2]

This should be defined inline in the sqlite_stmt otherwise this wont compile due to incomplete types.

[pfultz2]

What is this parameter? It looks like it duplicates the key in binary_cache_entry. If so, it should be removed.

[pfultz2]

This should take the key and the backends can use a hash if necessary.

[pfultz2]

Declare size as std::size_t and drop the static_cast:

Suggested change

	auto size = data == nullptr ? 0 : static_cast<std::size_t>(bytes);
	std::size_t size = data == nullptr ? 0 : bytes;

[pfultz2]

These modernize-return-braced-init-list tidy warning are little annoying here. Another option instead of suppressing them is to add a pair function to the value class:

struct MIGRAPHX_EXPORT value
{
    ...
    template<class... Ts>
    static pair(const std::string& pkey, Ts&&... xs)
    {
        return value(pkey, static_cast<Ts&&>(xs...));
    }
    ...
};

Then you can add whatever suppressions that are needed in the pair function, and then just use value::pair instead of value and there shouldn't be tidy warnings for everytime.

[pfultz2]

Why use the intermediate vector? Just insert directly into the returned value:

Suggested change

	std::vector<value> columns;
	value columns = value::object{};

[Copilot]

Copilot review overview

🟡 Changes recommended

Release-mode validation holes, statement cleanup issues, and the missing promised provenance table remain unresolved.

Review effort: Balanced
Findings: 3 · 3

Open (6)

• Reset statement before rethrowing initial step exception · New
• Reject empty SQL when sqlite3_prepare returns no statement · New
• A failed write leaves store_stmt valid, so every later kernel retries the same unusable database.…
• Implement or revise missing cache_info_v1 provenance table · New
• This removes the directory-backed disk-hit, corruption, compile-twice, and verified-reuse…
• The new extension-based backend selection is not reflected in the binary-cache documentation:…

Resolved since last review (2)

• Opening exclusively through try_write() makes the documented read-only fallback unreachable for…
• The constructor now computes version_dir() even when persistence is disabled. Since every GPU…