cairo_native icon indicating copy to clipboard operation
cairo_native copied to clipboard

Ensure that all `llvm.alloca`s are on the helper's init block.

Open azteca1998 opened this issue 1 year ago • 10 comments

All llvm.alloca (and their dependencies, which should be constants only) should be appended to the init block, otherwise the tail recursion mechanism will keep allocating more and more stack space every iteration, which is precisely what we want to avoid by using that mechanism.

azteca1998 avatar Apr 23 '24 17:04 azteca1998

Let me know if i can work in this one.

Gerson2102 avatar Apr 23 '24 18:04 Gerson2102

Hey, whats up! I'm a first time contributor and would love to introduce myself in the codebase. Let me know if I can grab this or any other issue :)

rvalenciano avatar Apr 24 '24 21:04 rvalenciano

@azteca1998 Hello mate. When you say llvm.alloca you mean llvm::alloca? Do i have to check in libfuncs for those things?

All llvm::alloca should be like this right?:

let stack_ptr = helper
                .init_block()
                .append_operation(llvm::alloca(
                    context,
                    value_len,
                    llvm::r#type::opaque_pointer(context),
                    location,
                    AllocaOptions::new()
                        .align(Some(IntegerAttribute::new(
                            IntegerType::new(context, 64).into(),
                            inner_layout.align() as i64,
                        )))
                        .elem_type(Some(TypeAttribute::new(inner_ty))),
                ))
                .result(0)?
                .into();

And i dont get the part that you say "and their dependencies".

Gerson2102 avatar Apr 26 '24 15:04 Gerson2102

Hello,

With llvm.alloca I'm referring to the alloca instructions in the llvm dialect. In MLIR they're written as llvm.alloca, but we have them available as llvm::alloca in Rust.

The llvm.alloca instruction accepts other values as arguments (which are dependencies). In your example, the argument would be value_len. They also need to be on the init block, otherwise the generated MLIR would be invalid.

All llvm.alloca operations must be in the init block instead of entry or whatever other local block every libfunc has. I know there's at least one place where it's not in the init block.

azteca1998 avatar Apr 29 '24 10:04 azteca1998

Hello,

With llvm.alloca I'm referring to the alloca instructions in the llvm dialect. In MLIR they're written as llvm.alloca, but we have them available as llvm::alloca in Rust.

The llvm.alloca instruction accepts other values as arguments (which are dependencies). In your example, the argument would be value_len. They also need to be on the init block, otherwise the generated MLIR would be invalid.

All llvm.alloca operations must be in the init block instead of entry or whatever other local block every libfunc has. I know there's at least one place where it's not in the init block.

In the previous example, llvm::alloca and the dependencie, are in the init block right? Because I'm using the helper where is located the init function.

When you say entry, is the variable we use to call init_block() right? And we dont want that, we want the LibFuncHelper, am I right?

Gerson2102 avatar Apr 29 '24 14:04 Gerson2102

helper.init_block() is the block at which the operation is appended. We want to append the init block (helper.init_block()) instead of entry (the default block for each libfunc), or any other block internally created within the libfunc (only present in complex libfuncs).

Basically, every .append_operation(llvm::alloca(...)) and its dependencies (the constant) should append to helper.init_block().

azteca1998 avatar Apr 29 '24 14:04 azteca1998

Ok. And i have to look through all the code of the project, not just the libfuncs directory?

Gerson2102 avatar Apr 29 '24 14:04 Gerson2102

Ideally yes, but outside the libfuncs (and a few helper functions I think) you won't have the helper. If you find something outside libfuncs' implementations just letting us know is fine, we'll fix it later.

azteca1998 avatar Apr 29 '24 14:04 azteca1998

This is already done

edg-l avatar May 16 '24 10:05 edg-l

This is already done

In my PR or someone else did this? @edg-l

Gerson2102 avatar May 16 '24 13:05 Gerson2102