Skip to content
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.

Sign up for GitHub

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

Jump to bottom

common, xe: consolidate serialization API #2802

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

common, xe: consolidate serialization API #2802

wants to merge 1 commit into from

Conversation

echeresh
Copy link
Contributor

@echeresh echeresh commented Mar 3, 2025 •
edited
Loading

PR consolidates serialization API in src/gpu/intel and src/common.

List of changes:

  • Moved serialized_t/serialized_data_t/deserializer_t functionality from src/gpu/intel to src/common
    • Merged serialized_data_t, serialized_t and serialization_stream_t functionality into serialization_stream_t
  • Renamed src/common/{serialization_stream.hpp => serialization.hpp} as it now includes broad serialization API (not just stream)
  • Renamed src/common/{serialization.hpp => primitive_serialization.hpp}

I'm not 100% sure on the names so please share your suggestions if any.

I'm also listing API changes to simplify/unify serialization API for review:

  • Combine serialized_t and serialized_data_t into a single abstraction (implemented in this PR)
  • Unify serialize/deserialize member API, stick to obj.serialize(stream). Now there are two versions: see (1) below
  • Introduce non-member functions for serialization/deserialization:
    • serialize(sstream, obj)
    • obj = deserialize<Object>(deserializer) or deserialize(deserializer, obj)
  • Rename sstream.append() to sstream.serialize() to have a single name
  • Rename deserializer.pop() to deserializer.deserialize() to have a single name
  • Introduce non-member deserialize(sstream, obj) which would construct a temporary deserializer_t on-the-fly and forward the call to deserialize(deserializer, obj)

(1)

$ grep 'void serialize(serialized_data' src -rIn
src/gpu/intel/gpu_post_ops.hpp:267:    void serialize(serialized_data_t &s) const {
src/gpu/intel/gpu_post_ops.hpp:309:    void serialize(serialized_data_t &s) const {
src/gpu/intel/gpu_post_ops.hpp:536:        void serialize(serialized_data_t &s) const {
src/gpu/intel/gpu_post_ops.hpp:597:    void serialize(serialized_data_t &s) const { s.append(ops_); }
src/gpu/intel/jit/conv/model.hpp:602:    void serialize(serialized_data_t &s) const { s.append(buckets_); }
src/gpu/intel/jit/conv/model.hpp:735:    void serialize(serialized_data_t &s) const {
src/gpu/intel/jit/conv/model.hpp:1083:    void serialize(serialized_data_t &s) const {
src/gpu/intel/jit/gemm/include/strategy.hpp:408:    void serialize(serialized_data_t &s) const
src/gpu/intel/jit/gemm/include/problem.hpp:268:    void serialize(serialized_data_t &s) const
src/gpu/intel/serialization.hpp:78:    // void serialize(serialized_data_t &) const
$ grep 'serialized_t serialize' src -rIn
src/gpu/intel/ocl/bnorm/reusable_bnorm.hpp:60:    serialized_t serialize() const {
src/gpu/intel/ocl/bnorm/nhwc_reusable.hpp:71:    serialized_t serialize() const {
src/gpu/intel/ocl/rnn/rnn_utils.hpp:175:    serialized_t serialize() const {
src/gpu/intel/ocl/gemm/xe_systolic_gemm_copy_kernel.hpp:126:    serialized_t serialize() const { return serialized_t(*this); }
src/gpu/intel/ocl/reusable_softmax.hpp:60:    serialized_t serialize() const {
src/gpu/intel/jit/v2/conv/kernel_desc.hpp:367:    serialized_t serialize() const override;
src/gpu/intel/jit/ir/kernel_desc.hpp:56:    virtual serialized_t serialize() const = 0;
src/gpu/intel/jit/conv/zero_out.hpp:52:    serialized_t serialize() const override;
src/gpu/intel/jit/gemm/gen_gemm_kernel.hpp:74:    serialized_t serialize() const { return serialized_t(problem_, strategy_); }
src/gpu/intel/serialization.hpp:252:    serialized_t serialize() const {

@echeresh echeresh added the platform:gpu-intel Codeowner: @oneapi-src/onednn-gpu-intel label Mar 3, 2025
@echeresh echeresh requested review from a team as code owners March 3, 2025 23:38
@echeresh echeresh mentioned this pull request Mar 4, 2025
Open
rjoursler
rjoursler reviewed Mar 5, 2025
View reviewed changes
src/common/primitive_serialization.hpp
namespace primitive_serialization {

void serialize_post_ops(
serialization_stream_t &sstream, const post_ops_t &post_ops);
Copy link
Contributor

@rjoursler rjoursler Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code seems excessively verbose and the style is unaligned with how the GPU serialization tends to work. What if we just use a templated function instead?

namespace  primitive {

template <typename T> void serialize(serialization_stream_t sstream, const T &t);

}

And then use extern template in this file. After that, usage is primitive::serialize(stream, data) which seems more readable. Debatably, we could also drop the namespace, it doesn't seem like it serves much of a purpose, as implementing multiple serializers for these core types seems like it would be an antipattern.

Copy link
Contributor Author

@echeresh echeresh Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, switching to overloading + dropping the namespace look reasonable to me.

@densamoilov please confirm if these changes are fine with you as I see you implemented this originally.

@echeresh
Copy link
Contributor Author

echeresh commented Mar 5, 2025

make test
set test_scope=NIGHTLY
disable benchdnn_all
enable benchdnn_conv
enable benchdnn_deconv
enable arch_gpu_xe-hpc

dzarukin
dzarukin approved these changes Mar 5, 2025
View reviewed changes
src/common/primitive_serialization.hpp
#include "common/primitive_attr.hpp"
#include "common/serialization.hpp"
#include "common/type_helpers.hpp"
#include "oneapi/dnnl/dnnl.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: probably can skip this one in favor of c_types_map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, thanks.

src/common/serialization.hpp
Comment on lines +265 to +266
size_t idx;
const serialization_stream_t &s;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make them private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

src/common/serialization.hpp
static const bool value = (sizeof(test<T>(0)) == sizeof(yes_t));
};

// Append helper function for structures with the member function
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: is append supposed to be public? Or should a ctor be the only user-facing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's supposed to be public as it's used as sstream.append(...) in many places.
I put some suggestions to unify API in the description (e.g. switch to uniform serialize(sstream, obj) usage) but I'd left it for the future.

@echeresh
Copy link
Contributor Author

echeresh commented Mar 8, 2025

make test
set test_scope=NIGHTLY
disable benchdnn_all
enable benchdnn_conv
enable benchdnn_deconv
enable arch_gpu_xe-hpc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjoursler rjoursler rjoursler left review comments

@dzarukin dzarukin dzarukin approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
platform:gpu-intel Codeowner: @oneapi-src/onednn-gpu-intel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@echeresh @dzarukin @rjoursler