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

460 builtin functions #511

Merged
merged 22 commits into from
Jul 25, 2022
Merged

460 builtin functions #511

merged 22 commits into from
Jul 25, 2022

Conversation

ghaith
Copy link
Collaborator

@ghaith ghaith commented Jul 4, 2022

More support for the built-in functions
Added a {sized} pragma for the varargs which when used will make sure the functions receive a sized argument and a pointer to the varargs, it then passes the size into the call.
This allows externally implemented functions to receive a pointer to the parameter data and loop through it.

Added MUX and SEL as built-in functions.

@ghaith ghaith linked an issue Jul 4, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2022

Codecov Report

Merging #511 (26ef2ab) into master (e2f1968) will increase coverage by 0.11%.
The diff coverage is 95.74%.

@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
+ Coverage   93.04%   93.15%   +0.11%     
==========================================
  Files          40       41       +1     
  Lines       14592    14838     +246     
==========================================
+ Hits        13577    13823     +246     
  Misses       1015     1015              
Impacted Files Coverage Δ
src/tests/adr/pou_adr.rs 100.00% <ø> (ø)
src/codegen/generators/llvm.rs 88.77% <55.55%> (-1.60%) ⬇️
src/typesystem.rs 95.74% <77.77%> (-0.52%) ⬇️
src/validation.rs 94.17% <80.00%> (+2.54%) ⬆️
src/resolver.rs 96.62% <93.06%> (+0.12%) ⬆️
src/builtins.rs 87.91% <94.54%> (+9.53%) ⬆️
src/resolver/generics.rs 96.38% <96.38%> (ø)
src/index/visitor.rs 96.37% <97.72%> (-0.14%) ⬇️
src/ast.rs 93.07% <100.00%> (+0.18%) ⬆️
src/ast/pre_processor.rs 99.23% <100.00%> (+<0.01%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2f1968...26ef2ab. Read the comment docs.

ghaith and others added 7 commits July 4, 2022 08:43
If one of the types is generics (because it was builtin)
it will not be cast.
Added the variadics to the index members to allow the resolver
to correctly address them / modify them once a generic type needs to be
replaced
@ghaith ghaith requested a review from riederm July 11, 2022 05:55
@ghaith
Copy link
Collaborator Author

ghaith commented Jul 11, 2022

In one of the commits, I added the possibility to have a "transformation" function that will spawn out new (replacement) ast. The idea is to run it in the preprocessor phase. I'm not sure how useful it will be though, it turned out I did not really need it yet.

Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

first half of the review
continue at generics.rs

src/builtins.rs Outdated Show resolved Hide resolved
src/builtins.rs Outdated
transformation: |it| it,
code: |generator, params, location| {
//Generate an access from the first param
if let &[k, ..] = params {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can deconstruct this in one go:

if let &[k, params..] = params {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is not yet stable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i changed it to if let (&[k], params) = params.split_at(1) {

src/builtins.rs Outdated Show resolved Hide resolved
src/builtins.rs Outdated Show resolved Hide resolved
src/codegen/generators/expression_generator.rs Outdated Show resolved Hide resolved
Comment on lines +787 to +798
let arr_storage = self.llvm.builder.build_alloca(arr, "");
for (i, ele) in generated_params.iter().enumerate() {
let ele_ptr = self.llvm.load_array_element(
arr_storage,
&[
self.llvm.context.i32_type().const_zero(),
self.llvm.context.i32_type().const_int(i as u64, true),
],
"",
)?;
self.llvm.builder.build_store(ele_ptr, *ele);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

llvm supports constant-arrays. I think runtime performance would be better

@X = global i32 17
@Y = global i32 42
@Z = global [2 x i32*] [ i32* @X, i32* @Y ]

https://llvm.org/docs/LangRef.html#global-variable-and-function-addresses

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not work on non-constant variables (expressions that need to be resolved) it was the initial implementation and I had to scrape it

Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you think so? did you try?
the inkwell type's are a bit opinionated. I just tried and at least I get the IR for the test builtin_function_call_mux generated:

   15    15 │   %load_c = load i32, i32* %c, align 4
   16    16 │   %load_d = load i32, i32* %d, align 4
   17    17 │   %load_e = load i32, i32* %e, align 4
   18    18 │   %1 = alloca [4 x i32], align 4
- 19        │-  %2 = getelementptr inbounds [4 x i32], [4 x i32]* %1, i32 0, i32 0
- 20        │-  store i32 %load_b, i32* %2, align 4
- 21        │-  %3 = getelementptr inbounds [4 x i32], [4 x i32]* %1, i32 0, i32 1
- 22        │-  store i32 %load_c, i32* %3, align 4
- 23        │-  %4 = getelementptr inbounds [4 x i32], [4 x i32]* %1, i32 0, i32 2
- 24        │-  store i32 %load_d, i32* %4, align 4
- 25        │-  %5 = getelementptr inbounds [4 x i32], [4 x i32]* %1, i32 0, i32 3
- 26        │-  store i32 %load_e, i32* %5, align 4
- 27        │-  %6 = getelementptr inbounds [4 x i32], [4 x i32]* %1, i32 0, i32 3
- 28        │-  %7 = load i32, i32* %6, align 4
- 29        │-  store i32 %7, i32* %a, align 4
+        19 │+  store [4 x i32] [i32 %load_b, i32 %load_c, i32 %load_d, i32 %load_e], [4 x i32]* %1, align 4
+        20 │+  %2 = getelementptr inbounds [4 x i32], [4 x i32]* %1, i32 0, i32 3
+        21 │+  %3 = load i32, i32* %2, align 4
+        22 │+  store i32 %3, i32* %a, align 4
   30    23 │   ret void
   31    24 │ }

It just works for int_values here, so i think you just need a helper method, where you pass the generated variadic parameters and it creates you an int_array or a ptr_array or a float_array etc. (depending on the variadic values you passed it --> see that all variadic parameters need to be the same type-nature anyway)

I think its a cleaner implementation and faster at runtime (and much easier for the optimizer)

here is my change:

index 9487bb518..893e72506 100644
--- a/src/codegen/generators/expression_generator.rs
+++ b/src/codegen/generators/expression_generator.rs
@@ -777,22 +777,19 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> {
             } else {
                 self.llvm_index.get_associated_type(type_name)
             }?;
-            let size = generated_params.len();
-            let size_param = self.llvm.i32_type().const_int(size as u64, true);
-            let arr = Llvm::get_array_type(llvm_type, size as u32);
-            let arr_storage = self.llvm.builder.build_alloca(arr, "");
-            for (i, ele) in generated_params.iter().enumerate() {
-                let ele_ptr = self.llvm.load_array_element(
-                    arr_storage,
-                    &[
-                        self.llvm.context.i32_type().const_zero(),
-                        self.llvm.context.i32_type().const_int(i as u64, true),
-                    ],
-                    "",
-                )?;
-                self.llvm.builder.build_store(ele_ptr, *ele);
-            }
-            Ok(vec![size_param.into(), arr_storage.into()])
+            // generate the variadic values
+            let size_value = self.llvm.i32_type().const_int(generated_params.len() as u64, false);
+            let arr_storage = self.llvm.builder.build_array_alloca(llvm_type, size_value, "");
+            // these 2 lines need a helper function (it now assumes intValues)
+            let values: Vec<IntValue> = generated_params
+                .iter()
+                .map(|it| it.into_int_value())
+                .collect();
+            self.llvm.builder.build_store(arr_storage, llvm_type.into_int_type().const_array(values.as_slice()));
+
+            let size_param = self.llvm.builder.build_alloca(self.llvm.i32_type(), "");
+            self.llvm.builder.build_store(size_param, size_value);
+           Ok(vec![size_param.into(), arr_storage.into()])
         } else {
             Ok(generated_params)
         }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There should be a correctness test for it. It will segfault or at least my implementation segfaults.
llialso refused to run it.
I didn't check your version yet maybe it's different I'll check tomorrow

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm strange. in the given example [ i32* @X, i32* @Y ] X and Y are also not const

src/codegen/generators/pou_generator.rs Outdated Show resolved Hide resolved
src/builtins.rs Outdated Show resolved Hide resolved
src/codegen/tests/expression_tests.rs Outdated Show resolved Hide resolved
src/lexer/tests/lexer_tests.rs Show resolved Hide resolved
@ghaith
Copy link
Collaborator Author

ghaith commented Jul 14, 2022

@riederm I would like to introduce an extra built in property here that allows us to override the resolver for a specific built in, this allows us to derive correct return types (oscat is having issue with cases like SEL(a,b,c) + 1) and also makes the codegen a bit simpler where I don't have to add special cases for non annotated generics like in the expression generator
This is bringing back that "transform" field, but not on the AST level, but on the resolver instead. What do you think about it?
I'm thinking of an optional resolver function, and if it exists we run it instead of the standard resolve for that call statement.

ghaith and others added 6 commits July 14, 2022 08:38
The MUX, SEL and MOVE functions will have their own annotations
This allows more control on the return types of the functions
The return type for builtins is now correctly annotated,
the special annotation is not yet used, will be needed for EXPT
Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

+1 after walkthrough with ghaith

Comment on lines +787 to +798
let arr_storage = self.llvm.builder.build_alloca(arr, "");
for (i, ele) in generated_params.iter().enumerate() {
let ele_ptr = self.llvm.load_array_element(
arr_storage,
&[
self.llvm.context.i32_type().const_zero(),
self.llvm.context.i32_type().const_int(i as u64, true),
],
"",
)?;
self.llvm.builder.build_store(ele_ptr, *ele);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm strange. in the given example [ i32* @X, i32* @Y ] X and Y are also not const

@ghaith ghaith merged commit 1abe861 into master Jul 25, 2022
@ghaith ghaith deleted the 460-builtin-functions branch July 25, 2022 07:49
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.

Builtin functions
3 participants