Experimental serializer #7594

TH3CHARLie · 2023-05-30T02:43:40Z

An attempt to add serialization/deserialization to the Halide frontend. Using flatbuffers (https://flatbuffers.dev/) as the binary format.

Work in progress right now, will summarize all unsupported features/design decisions in a bit.

Shout out to @zvookin 's previous attempt (main...halide_ir_flatbuffer) which I take heavy reference from.

… Dag yet

tested

steven-johnson

This looks like a really nice start! My comments are mostly regarding code style and structure. I suspect @zvookin will have more substantive comments about the Flatbuffer usage, as he has far more experience than I with that.

apps/serdes/Deserializer.h

steven-johnson · 2023-05-30T22:03:32Z

apps/serdes/Deserializer.cpp

+#include <fstream>
+#include <iostream>
+
+std::string Deserializer::deserialize_string(const flatbuffers::String *str) {


If str can't ever (legally) be null, we should pass it by const-ref rather than const-ptr.

apps/serdes/Deserializer.h

apps/serdes/Deserializer.cpp

apps/serdes/Printer.h

apps/serdes/Serializer.cpp

steven-johnson · 2023-05-30T22:34:22Z

apps/serdes/Serializer.cpp

+ case Halide::Internal::ForType::GPULane: {
+ for_type = Halide::Serdes::ForType::ForType_GPULane;
+ break;
+ }


Here and elsewhere: how do we ensure that additions to one of the enums (in either the .fbs or the Halide headers files) are correctly handled? I don't know the answer to this (and I'm not expecting you to have the answer, either), but I hope there exists an answer that is more robust than "hope we don't forget someday".

I think we can alleviate this by removing the default case of the switch statement, and do the error handling outside of the switch. Then whenever we have a new addition to the enums in Halide headers (I think the workflow will be starting with adding to Halide files and then serialization protocol fbs file), the compiler will give an error here because of non-exhaustive enumeration

apps/serdes/halide_ir.fbs

steven-johnson · 2023-05-30T22:37:48Z

src/Function.h

@@ -68,6 +68,10 @@ class Function {
 /** Construct a Function from an existing FunctionContents pointer. Must be non-null */
 explicit Function(const FunctionPtr &);

+ /** Construct a function from deserializing */


The arguments to this need documentation:

How is origin_name different from name?

How are required_types different from output_types?

What kind of strings are ok for args?

What happens if I pass empty vectors for any of the arguments?

TH3CHARLie · 2023-05-31T01:02:18Z

@steven-johnson Thanks for the review comments! I'm still merging some code related to function schedules, I'll have a commit later addressing the comments.

- proper using - proper includes - rename Serdes -> Serialize

- rename .hlb/.hlr to .hlpipe - reserve vectors - proper memory management

steven-johnson

Finally LGTM pending green.

…Halide into pr/7594

steven-johnson · 2023-08-07T16:26:19Z

Just waiting for @alexreinking to approve since there are pending "requested changes"

steven-johnson · 2023-08-11T18:34:37Z

Stil waiting for @alexreinking to approve since there are pending "requested changes"

alexreinking · 2023-08-11T21:27:40Z

src/CMakeLists.txt

+ # fail on case-sensitive file systems. We'll try this as a hack workaround:
+ # just try all three. (Note that the internal CMake library name appears to be
+ # `flatbuffers` in all cases.)
+ set(FB_NAME "")


Have you considered using the NAMES argument to find_package? That would simplify this considerably

...because I didn't know about it?

Sorry, sometimes I forget the abyss stared back into me

alexreinking · 2023-08-11T21:28:21Z

src/CMakeLists.txt

+ VERBATIM
+ )
+ add_custom_target(generate_fb_header DEPENDS "${fb_header}")
+ set_source_files_properties("${fb_header}" PROPERTIES GENERATED TRUE)


Why is this necessary? Add custom command sets it automatically

I didn't realize that.

alexreinking · 2023-08-11T21:29:55Z

src/CMakeLists.txt

+
+# flatbuffers is small and compiles quickly, but if you want/need to use
+# a local version (via find_package), configure with FLATBUFFERS_USE_FETCHCONTENT=OFF
+option(FLATBUFFERS_USE_FETCHCONTENT "Enable to download the Flatbuffers library via FetchContent" ON)


This whole FetchContent thing makes me sad. We'll (I'll) be able to do much better with CMake 3.24+ and its tighter integration between the two methods.

Actually, an upgrade would give us so much leverage that I think we should poll our users to see if they can tolerate an early upgrade to 3.24.

alexreinking

On second thought, I can fix the NAMES thing in a follow up. Let's land this.

TH3CHARLie · 2023-08-12T01:34:25Z

Thanks everyone for the help!

abadams · 2023-08-15T20:00:45Z

src/Pipeline.h

 std::vector<Argument> infer_arguments(const Internal::Stmt &body);

 /** Get the Funcs this pipeline outputs. */
 std::vector<Func> outputs() const;

+ /** Get the requirements of this pipeline. */
+ std::vector<Internal::Stmt> requirements() const;


This should probably return a const ref to a vector

abadams · 2023-08-15T20:01:50Z

src/Pipeline.h

@@ -184,11 +184,17 @@ class Pipeline {
 * outputs. Schedules the Funcs compute_root(). */
 Pipeline(const std::vector<Func> &outputs);

+ /** Make a pipeline from deserialization. */


This comment is confusing, because the args are not a "deserialization". A better comment would be something like: "Make a Pipeline with a vector of outputs and a list of requirement assertions. Used by deserialization."

abadams · 2023-08-15T20:02:09Z

src/Reduction.h

@@ -54,6 +54,9 @@ class ReductionDomain {
 * the vector being outermost. */
 ReductionDomain(const std::vector<ReductionVariable> &domain);

+ /** Construct a reduction domain from deserialization */


This comment isn't very good either

abadams · 2023-08-15T20:03:15Z

src/Schedule.h

@@ -652,6 +669,10 @@ class StageSchedule {
 }
 StageSchedule(const StageSchedule &other) = default;
 StageSchedule();
+ StageSchedule(const std::vector<ReductionVariable> &rvars, const std::vector<Split> &splits,


Needs a comment explaining what this constructor is for

abadams · 2023-08-15T20:03:32Z

src/Serialization.h

+
+namespace Halide {
+
+void serialize_pipeline(const Pipeline &pipeline, const std::string &filename, std::map<std::string, Internal::Parameter> &params);


Badly needs a long comment explaining serialization.

abadams · 2023-08-15T20:04:09Z

src/Deserialization.h

+namespace Halide {
+
+/**
+ * Deserialize a Halide pipeline from a file.


This comment needs to be longer, explaining in more detail what the map is for.

abadams · 2023-08-15T20:04:17Z

src/Deserialization.h

+ */
+Pipeline deserialize_pipeline(const std::string &filename, const std::map<std::string, Internal::Parameter> &external_params);
+
+Pipeline deserialize_pipeline(std::istream &in, const std::map<std::string, Internal::Parameter> &external_params);


This function needs a doc string

abadams · 2023-08-15T20:05:20Z

src/Parameter.h

@@ -23,6 +23,11 @@ namespace Internal {

 struct ParameterContents;

+struct BufferConstraint {


Moving this into the header moved into the public API. It needs documentation that explains what each field is and explains that it's only for serialization.

abadams · 2023-08-15T20:05:55Z

src/Parameter.h

+ const Buffer<void> &buffer, int host_alignment, const std::vector<BufferConstraint> &buffer_constraints,
+ MemoryType memory_type);
+
+ Parameter(const Type &t, bool is_buffer, int dimensions, const std::string &name,


Needs a doc string. Note that in doxygen output this won't necessarily appear in a way that makes it clear that the comment on the previous constructor also applies.

abadams · 2023-08-15T20:07:40Z

If I understand correctly, there's going to be a follow-up PR that adds testing and fixes any issues that adding testing finds. Please also address my review comments above in that follow-up PR.

TH3CHARLie · 2023-08-15T20:09:17Z

I can send a PR today adding the JIT test we talked about in the dev meeting, enclosed in #ifdef blocks. Some docstring are addressed in Derek's recent PR #7760

* init * sync * single func pipeline round-trip test * roundtrip test framework completed, single output function tested, no Dag yet * serialize Stmt, partially done (cuz no support of Expr yet), not fully tested * deviceAPI MemoryType ForType * Expr, with a grain of salt * fix exprs in stmts * format everything * Range * fix undefined exprs and stmts * address some review comments: - proper using - proper includes - rename Serdes -> Serialize * address more review comments - rename .hlb/.hlr to .hlpipe - reserve vectors - proper memory management * deserialize_expr_vector * support bound, storageDim, loopLevel and funcSchedule * Specialization, Definition * sync commit * temporarily comment out func mapping stuff to remove blockers * helper funcs * call_type and reduction_domain * ModulusRemainder and VectorReduceOp, some minor refactoring * prefetch directive * name mangling and closing on function's odds and ends * split * dim * stage schedule * tidy * parameter * more parameter * check nullptr and some minor fix * fix crashing * func index replacing func ptr during serialization * extern func arg, some minor cleanup * replace cerr with halide assert * buffer?? * remove printer * fix * wrappers in func_schedule * clear func mapping to use serializer for more than 1 pipelines, use unordered_map also * attempt to move serialization into core, get cmake working for now * fix * we maybe don't need submodule * fix cmake * make headers work again, with some hacks ofc * serialization now lives in libHalide * testing 101 * don't include flatbuffers header in Halide.h * fix * namespace adjust * user_assert * fix a missing field * fix missing type info in some exprs * fix bug in function mapping * fix function DAG broken issue * format * rm cout in cpp files and change test group name * fix the case func ptr is not defined * add a missing call type deserialization * serialize unique parameters * serialize unique buffers * fix missing type in parameter * fix a missing tail stra * change find_transive_call to build_enviroment to include wrappers in the DAG * upstream current test strategy, intercept JIT compilation for each pipeline, serdes ronudtrip and back * make sure buffer memory layout are the same * don't use ir comparator to compare pipelines, we will use jit tests from now * don't serialize Parameter's buffer, compute external buffers from Call, Variable and ExternFuncArgument and don't serialize them as well * fix, 35 tests remaining * fix output function orders * reuse jit_externs since we cannot really serialize it, 29 tests to go * fix that buffer_constraints, host_alignment and memory_type are incorrectly removed, also add missing exact in split * only use outputs and requirements from deserialized pipeline during testing * nits * add missing requirements during deserialization * restore original pipeline's contents after lowering * address some review comments * Install flatbuffers for clang-tidy * use std::map to make results the same on different compiler * proper way to handle cropped buffers * fix cmake build using alex's branch * try set flatbuffers_DIR explicitly * case sensitive? * rename serialization test env var * cleanup Serialization.cpp * format * have halide version embedded in the file identifier * nits and comments * format * try make clang-tidy happy and const a lot of things * const more things * support istream input * nit * add template function deserialize_vector * nit * attempt to integrate serialization test * line breaks * remove hack in compile_jit, at least for now * fix * add #ifdef guards * format * try nolint * special case two files so clang-tidy will be happy * Make Flatbuffers-missing error more useful * Make a few final changes - change BUILD_SERIALIZATION -> WITH_SERIALIZATION to match other flags better - fix capitalization of the CMake package (must be `FlatBuffers` for some Linux usage) - add stub calls to the de/serialization calls when building without Flatbuffers * Oops addition * clang-format * Add temporary debug hackery * more hackery * grr * sdfsdf * sigh, capitalization * One more try * Update presubmit.yml * No more mr nice guy * Update CMakeLists.txt * Revise build rules & script to allow clang-tidy for the new files * Update CMakeLists.txt * Apply clang-tidy fixes * Fix target for generated header * Prefer to use FetchContent for flatbuffers * Fixes * set PIC on * more pic * fix attempt * fix attempt * try macos * coreutils * Update run-clang-tidy.sh * noquiet * final again? --------- Co-authored-by: Steven Johnson <srj@google.com>

TH3CHARLie added 11 commits March 22, 2023 16:47

init

7bafefc

sync

10fce24

single func pipeline round-trip test

acd6223

Merge branch 'main' into xuanda/serialization

937cb48

roundtrip test framework completed, single output function tested, no…

256c36d

… Dag yet

serialize Stmt, partially done (cuz no support of Expr yet), not fully

7804953

tested

deviceAPI MemoryType ForType

dd6d9fc

Expr, with a grain of salt

e3a55c5

fix exprs in stmts

4c19481

format everything

dfff1a9

Range

6838258

steven-johnson requested a review from zvookin May 30, 2023 16:12

steven-johnson reviewed May 30, 2023

View reviewed changes

TH3CHARLie added 16 commits June 5, 2023 16:09

fix undefined exprs and stmts

d6b0b00

address some review comments:

609705d

- proper using - proper includes - rename Serdes -> Serialize

address more review comments

4b86347

- rename .hlb/.hlr to .hlpipe - reserve vectors - proper memory management

deserialize_expr_vector

35296ff

support bound, storageDim, loopLevel and funcSchedule

fe79618

Specialization, Definition

478970e

sync commit

4140ae1

temporarily comment out func mapping stuff to remove blockers

df3620b

helper funcs

b416d26

call_type and reduction_domain

db47e2c

ModulusRemainder and VectorReduceOp, some minor refactoring

63ac57a

prefetch directive

ca4ac94

name mangling and closing on function's odds and ends

097ec2e

split

88f4a5d

dim

491b596

stage schedule

b7292b7

steven-johnson added 7 commits August 4, 2023 21:24

fix attempt

934e3f9

fix attempt

4f2c6e4

try macos

568a740

coreutils

8bf2d67

Update run-clang-tidy.sh

df0addd

noquiet

e9fd2aa

final again?

1f9cf57

steven-johnson approved these changes Aug 4, 2023

View reviewed changes

steven-johnson added 2 commits August 4, 2023 16:37

Merge branch 'main' into pr/7594

3ff9fd3

Merge branch 'xuanda/serialization' of https://github.com/TH3CHARLie/…

cc1eec5

…Halide into pr/7594

alexreinking requested changes Aug 11, 2023

View reviewed changes

steven-johnson mentioned this pull request Aug 11, 2023

Fix some CMake issues regarding Flatbuffers #7757

Closed

3 tasks

alexreinking approved these changes Aug 11, 2023

View reviewed changes

steven-johnson merged commit f75f68d into halide:main Aug 11, 2023
3 checks passed

abadams reviewed Aug 15, 2023

View reviewed changes

BrewTestBot mentioned this pull request Feb 2, 2024

halide 17.0.0 Homebrew/homebrew-core#161602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimental serializer #7594

Experimental serializer #7594

TH3CHARLie commented May 30, 2023

steven-johnson left a comment

steven-johnson May 30, 2023

steven-johnson May 30, 2023

TH3CHARLie May 31, 2023

steven-johnson May 30, 2023

TH3CHARLie commented May 31, 2023

steven-johnson left a comment

steven-johnson commented Aug 7, 2023

steven-johnson commented Aug 11, 2023

alexreinking Aug 11, 2023

steven-johnson Aug 11, 2023

alexreinking Aug 11, 2023

alexreinking Aug 11, 2023

steven-johnson Aug 11, 2023

alexreinking Aug 11, 2023

alexreinking Aug 11, 2023

steven-johnson Aug 11, 2023

alexreinking left a comment

TH3CHARLie commented Aug 12, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams Aug 15, 2023

abadams commented Aug 15, 2023

TH3CHARLie commented Aug 15, 2023


		namespace Halide {

		void serialize_pipeline(const Pipeline &pipeline, const std::string &filename, std::map<std::string, Internal::Parameter> &params);

		@@ -23,6 +23,11 @@ namespace Internal {

		struct ParameterContents;

		struct BufferConstraint {

Experimental serializer #7594

Experimental serializer #7594

Conversation

TH3CHARLie commented May 30, 2023

steven-johnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TH3CHARLie commented May 31, 2023

steven-johnson left a comment

Choose a reason for hiding this comment

steven-johnson commented Aug 7, 2023

steven-johnson commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreinking left a comment

Choose a reason for hiding this comment

TH3CHARLie commented Aug 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadams commented Aug 15, 2023

TH3CHARLie commented Aug 15, 2023