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.

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

CubeCL first iteration #1756

Merged
merged 60 commits into from May 15, 2024
Merged

CubeCL first iteration #1756

merged 60 commits into from May 15, 2024

Conversation

louisfd
Copy link
Member

@louisfd louisfd commented May 11, 2024

Part of #1665

Since the goal of CubeCL is to rewrite all our JIT kernels with a readable rusty syntax, I started by supporting all the basic syntax to parse it into to the JIT intermediate representation, and figured out an enjoyable type system for element types, with support for easy casting. To look at examples, navigate to crates/burn-cube/tests where kernels marked with #[cube] are tested against their gpu! macro counterparts.

Next steps:

  • rename gpu macro to cube_inline (trivial, but didn't want to make this PR harder to read)
  • support vectorization better
  • migrate jit codegen stuff to cube

Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 78.13688% with 460 lines in your changes are missing coverage. Please review.

Project coverage is 86.41%. Comparing base (1073752) to head (2bf7db9).
Report is 6 commits behind head on main.

Files Patch % Lines
crates/burn-cube/tests/cast_elem.rs 58.16% 64 Missing ⚠️
crates/burn-cube/src/operation/binary.rs 49.41% 43 Missing ⚠️
crates/burn-cube/tests/function_call.rs 68.60% 27 Missing ⚠️
crates/burn-cube/tests/trait.rs 81.61% 25 Missing ⚠️
crates/burn-cube-macros/src/analysis.rs 85.71% 24 Missing ⚠️
crates/burn-cube-macros/src/codegen/function.rs 65.67% 23 Missing ⚠️
crates/burn-cube/src/element/conversion.rs 0.00% 22 Missing ⚠️
crates/burn-cube/src/element/bool.rs 25.00% 18 Missing ⚠️
crates/burn-cube/src/element/uint.rs 40.00% 18 Missing ⚠️
crates/burn-cube/src/operation/cmp.rs 56.75% 16 Missing ⚠️
... and 22 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1756      +/-   ##
==========================================
- Coverage   86.61%   86.41%   -0.21%     
==========================================
  Files         700      734      +34     
  Lines       83423    85575    +2152     
==========================================
+ Hits        72258    73947    +1689     
- Misses      11165    11628     +463     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

/// Derive macro for the module.
#[proc_macro_attribute]
pub fn cube(attr: TokenStream, tokens: TokenStream) -> TokenStream {
let args = parse_macro_input!(attr with Punctuated::<Meta, syn::Token![,]>::parse_terminated);
Copy link
Member

Choose a reason for hiding this comment

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

What is that 😮


pub struct CubeContext {
pub root: Rc<RefCell<Scope>>,
pub scope: Rc<RefCell<Scope>>,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to put the scope in a cell (Rc) if yes where do we clone it?

#[derive(new, Clone, Debug)]
/// Reference to a JIT variable
/// It's the expand element that is actually kept in the variable pool
pub struct ExpandElement {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the name, maybe CubeVariable would be better?

Comment on lines 8 to 14
+ std::cmp::PartialOrd
+ std::ops::Add<Output = Self>
+ std::ops::Sub<Output = Self>
+ std::ops::Mul<Output = Self>
+ std::ops::Div<Output = Self>
+ std::ops::AddAssign
+ std::ops::Rem<Output = Self>
Copy link
Member

Choose a reason for hiding this comment

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

Could all of those be moved to the Numeric trait ?

Comment on lines 5 to 14
pub trait RuntimeType: CubeType<ExpandType = ExpandElement> {
type Primitive;
fn val(&self) -> Self::Primitive;
fn into_elem() -> Elem;
fn from_expand(context: &mut CubeContext, val: ExpandElement) -> ExpandElement {
let new_var = context.create_local(Item::Scalar(<Self as RuntimeType>::into_elem()));
assign::expand(context, val, new_var.clone());
new_var
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We should add documentation on that trait, since it's so central.

@nathanielsimard nathanielsimard merged commit 542790e into main May 15, 2024
15 checks passed
@nathanielsimard nathanielsimard deleted the feat/cube-pl branch May 15, 2024 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants