Fusion executor

[csarofeen]

Reworking kernel.cpp to be more stand alone with more automation. Not sure yet if we'll replace it or just work on the ideas and refactor kernel.cpp

Still need:

• Broadcast support in launch config inference
• Global memory allocations (for reductions)
• Thread binding constraints on symbolic values
• Option to allocate outputs for you
• Line numbers on kernel code for error output
• Cache based on input run-time sizes
• Allow splitting on symbolic integer

[jjsjann123]

LGTM as an initial PR to start the conversation / refactor.

We could use a merge and adopt some of the changes to kernel.cpp.

I like the new launch config inference using existing fusion IR, (even tho I have some questions/concerns).

FYI: We have some tests where we are launching kernel with launch config that does not comply to the size of bound axes, (which may or may not be legit test case), but they would not be doable using this interface.

[jjsjann123]

Can we try merge dev branch and restore kernel.cpp?

[csarofeen]

Done, need to see what you've changed to figure out if I need to take some new things in.

[jjsjann123]

val is not used?

[csarofeen]

Thanks, meant to include it in the error message. Fixed.

[jjsjann123]

nitpick: use unique_ptr to make ownership clear?

[jjsjann123]

I like it that we are inferring binding/launch_config from the codegen IR.
My question here is, would the same thing work for things like: dynamic shared memory size & global buffer size?

[csarofeen]

Yes, this is precisely how we will do it. I think everything will go together so it can use the same expression context.

[jjsjann123]

I was discussing with @naoyam on this topic earlier.

I think it would be cleaner if we can have aggregated global buffer request and codegen split buffer appropriately. (same way that we would work with dynamic shared memory.) That we we can infer buffer size in the IR easier using ExpressionEvaluator.

[csarofeen]

I don't think we need split buffers, but we can allocate individual buffers. Yes, I am planning to do this.

[jjsjann123]

I'm asking a future-ish question here.

If we are to support broadcast of size-1 dimension, does that mean we could have an IterDomain bind to thread dimension, which has size-1, while non-broadcasted tensor binding to the same thread dimension would have a different size.

[csarofeen]

We're going to have to figure out how that will work. I don't have the logic in for parallelized broadcast right now. I think we'll just exclude size=1 dims from the inference.

[csarofeen]

Yes, this is precisely how we will do it. I think everything will go together so it can use the same expression context.

[csarofeen]

We're going to have to figure out how that will work. I don't have the logic in for parallelized broadcast right now. I think we'll just exclude size=1 dims from the inference.

[csarofeen]

I don't think we need split buffers, but we can allocate individual buffers. Yes, I am planning to do this.

[csarofeen]

Thanks, meant to include it in the error message. Fixed.

[csarofeen]

Done, need to see what you've changed to figure out if I need to take some new things in.

[csarofeen]

I removed the below as this is checked elsewhere (partially above)

[tlemo]

Looks like a nice step in the right direction.

[tlemo]

nit: is this needed?

[csarofeen]

🤷

[tlemo]

add a comment explaining what this does (and what a Philox is?)

[csarofeen]

Who doesn't know about Philox?! Will thoroughly comment when we're closer to usability.

[csarofeen]

changed var name to be more clear

[tlemo]

why the cast? did you mean
(unsigned int)(bdimx_ == -1 ? 1 : bdimx_) ?
or even better
static_cast<unsigned int>(bdimx_ == -1 ? 1 : bdimx_)

[csarofeen]

All syntax options here work, changed to static_cast.

[tlemo]

what is the meaning of -1 vs 1 ? maybe create symbolic constants for them?

[csarofeen]

Added a todo to convert them to c10::optional. Though even that seems like a relatively heavy hammer.

[tlemo]

Using -1 and 1 as special values may be ok, but they could use good symbolic names

[jjsjann123]

NITPICK: Is -1 even necessary?
In cases where we access un-initialized binding, we map -1 to 1 anyway. So maybe we can just stick with 1 to begin with as default binding and not use c10::optional?

[csarofeen]

Good point Jie, I was actually using both -1 and 1 as wildcard values. However, what I should be doing is using -1 as a wildcard value, and not evaluating broadcast dimensions. The broadcast dimensions were setting params to 1 which would be over-written later, but this isn't useful information to check self consistency in the bindings. I switched to -1 only, and if we get a value of 1 we assume that's a real inferred value.

[tlemo]

nit: not needed

[tlemo]

= false;

[tlemo]

So we don't mutate the input fusion? Nice! (the comment needs to be updated, there's no pointer in here)

[csarofeen]

Removed for now, still need to consider const model, but taking our own copy and not modifying it is probably enough.

[tlemo]

we can const Fusion* fusion !

[csarofeen]

Can't yet, need to go through Fusion and fix its constness model. This is a goal, but not high priority at the moment.

[tlemo]

since we make a copy, why not? (the copy constructor take a const Fusion&)

[tlemo]

nit: launch_params instead of the cryptic lp?

[tlemo]

kernel arguments instead of kah ?

[csarofeen]

uncomment fusion_id

[csarofeen]

uint_64 or int_64

[csarofeen]

done

[csarofeen]

Will add comments when it's closer to being usable.

[csarofeen]

Can't yet, need to go through Fusion and fix its constness model. This is a goal, but not high priority at the moment.

[csarofeen]

If you like kernel then why not compileKernel and runKernel? We are doing both, just lazy to remember name changes from one to the next.

[csarofeen]

I always liked append for pushing multiple values, but can change it to be more consistent. Always forget about standard containers.

[csarofeen]

Who doesn't know about Philox?! Will thoroughly comment when we're closer to usability.

[csarofeen]

All syntax options here work, changed to static_cast.

[csarofeen]

Added a todo to convert them to c10::optional. Though even that seems like a relatively heavy hammer.

[csarofeen]

surprised clang format didn't do that...

[tlemo]

case (0) vs case 0 :)

[tlemo]

compile implies a transformation: we compile a fusion into a kernel. The names should reflect that (ex. you compile c++ code and run the binary. if someone would say they run C++ code you'd probably think of an interpreter)

[tlemo]

consistency with standard containers is nice, but self-consistency is what I was going after (I think multiple overloads, all names either push or append would be fine)

[tlemo]

I did a quick pass and left some small comments.

This would be easier to review if it was split into multiple parts, but I guess we're past that point now?

[tlemo]

probably not intentional?

[tlemo]

yay! (I like this pattern better)

[tlemo]

please update the IrGraphGenerator test case to include this node type

[csarofeen]

This is done.

[tlemo]

please remove the parentheses

[tlemo]

remove ( )

[tlemo]

these deserve some comments

[tlemo]

move lowr() to a dedicated private section for members (please don't mix data member and methods)

[tlemo]

nit: the default path can be moved inside the switch/default

[tlemo]

->as() ?

[tlemo]

please use separate private sections for data member and methods (private methods, then private data)

[tlemo]

why is this removed? it was added to make sure the launch config is copied correctly

[csarofeen]

Launch config is being removed. Done in follow up PR.

[tlemo]

pls add coverage for GridReduction (same in FusionMove test case)

[csarofeen]

Fusion copy and fusion move tests are not exhaustively testing the IR. If you think that needs to be covered please open an issue and we can hit these tests harder.

[tlemo]

add a comment here?

[tlemo]

is this a local helper?

[csarofeen]

It's in an anonymous namespace, so yes.

[tlemo]

thanks. it's really hard to tell while reviewing a PR (which is a reason I prefer static instead of anonymous namespaces)

[tlemo]

maybe factor this out as a separate helper function?

[csarofeen]

Index compute may deserve a refactor but probably not in the most need for it at this point.

[tlemo]

this could be simplified as a unordered_map<string, ParallelType> lookup

[csarofeen]

We did use an unordered map, this was explicitly changed by FB due to an internal build failure.

[tlemo]

same

[csarofeen]

same

[tlemo]

const ref ?

[tlemo]

const ref

[csarofeen]

kernel cache will get rewritten or removed in following prs.

[tlemo]

I mean, why not assign directly to lowered_exprs_ ?

[tlemo]

Overall it looks good to me.

The only part I'm not sure about is the FusionExecutor interface: why do we a stateful, yet reusable executor?

[tlemo]

why are all these functions extern?

[csarofeen]

what would you prefer them to be?

[tlemo]

nit: empty line after #pragma once

[tlemo]

nit: = default?

[tlemo]

why aren't CompileOptions passed to compileFusion() ?

[csarofeen]

Yes, I changed this in: #182

[tlemo]

A FusionExecutor seems to be usable for multiple compilations / runs, right? At the same time, it's stateful. What is the rationale for this approach (as opposed to a more "functional" design - compile(fusion) -> kernel. kernel object can be executed)

[tlemo]

nice