Skip to content

Define intrinsic functions #2920

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

Merged
merged 3 commits into from
Jun 24, 2022
Merged

Define intrinsic functions #2920

merged 3 commits into from
Jun 24, 2022

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Jun 20, 2022

This fixes #2652 by implementing various compiler intrinsics as real functions. Also see #2821 (comment) and #2805.
I think it makes sense to move a few more functions over if possible, like the system call functions and runtime.supportsRecover. But this can be done at a later time.

Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

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

LGTM

@dgryski
Copy link
Member

dgryski commented Jun 23, 2022

@dgryski
Copy link
Member

dgryski commented Jun 23, 2022

Failure is due to the definition of the sync/atomic.Store* functions

; Function Attrs: minsize nounwind optsize
define hidden void @"sync/atomic.StoreInt32"(i32* dereferenceable_or_null(4) %addr, i32 %val, i8* %context) unnamed_addr addrspace(1) #1 !dbg !178 {
entry:
  call addrspace(1) void @llvm.dbg.value(metadata i32* %addr, metadata !180, metadata !DIExpression()), !dbg !182
  call addrspace(1) void @llvm.dbg.value(metadata i32 %val, metadata !181, metadata !DIExpression()), !dbg !182
  %0 = call addrspace(1) i32 @__atomic_store_4(i32* %addr, i32 %val, i16 5), !dbg !182
  ret i32 %0, !dbg !182
}

aykevl added 3 commits June 23, 2022 23:34
This changes the compiler from treating calls to sync/atomic.* functions
as special calls (emitted directly at the call site) to actually
defining their declarations when there is no Go SSA implementation. And
rely on the inliner to inline these very small functions.
This works a bit better in practice. For example, this makes it possible
to use these functions in deferred function calls.

This commit is a bit large because it also needs to refactor a few
things to make it possible to define such intrinsic functions.
This makes them available to deferred calls, among others.
This makes it possible to //go:linkname them from other places, like in
the reflect package. And is in my opinion a much cleaner solution.
@aykevl aykevl force-pushed the define-intrinsic-functions branch from 47d487b to c425be0 Compare June 23, 2022 21:35
@aykevl
Copy link
Member Author

aykevl commented Jun 23, 2022

Thank you for debugging! This should be fixed now.

@dgryski
Copy link
Member

dgryski commented Jun 23, 2022

Reran the tinyhci checks -- they're passing now.

Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

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

LGTM

@deadprogram
Copy link
Member

Thanks for working on this @aykevl and for all the reviewing @dgryski now merging.

@deadprogram deadprogram merged commit 4262f0f into dev Jun 24, 2022
@deadprogram deadprogram deleted the define-intrinsic-functions branch June 24, 2022 09:10
aykevl added a commit that referenced this pull request Aug 28, 2022
Instead of changing the calls, replace the function bodies themselves.
This is useful for a number of reasons, see
#2920 for more information.

I have removed the math intrinsics tests because they are no longer
useful. Instead, I think `tinygo test math` should suffice.
aykevl added a commit that referenced this pull request Aug 28, 2022
Instead of changing the calls, replace the function bodies themselves.
This is useful for a number of reasons, see
#2920 for more information.

I have removed the math intrinsics tests because they are no longer
useful. Instead, I think `tinygo test math` should suffice.
aykevl added a commit that referenced this pull request Aug 30, 2022
Instead of changing the calls, replace the function bodies themselves.
This is useful for a number of reasons, see
#2920 for more information.

I have removed the math intrinsics tests because they are no longer
useful. Instead, I think `tinygo test math` should suffice.
aykevl added a commit that referenced this pull request Aug 30, 2022
Instead of changing the calls, replace the function bodies themselves.
This is useful for a number of reasons, see
#2920 for more information.

I have removed the math intrinsics tests because they are no longer
useful. Instead, I think `tinygo test math` should suffice.
aykevl added a commit that referenced this pull request Aug 30, 2022
Instead of changing the calls, replace the function bodies themselves.
This is useful for a number of reasons, see
#2920 for more information.

I have removed the math intrinsics tests because they are no longer
useful. Instead, I think `tinygo test math` should suffice.
deadprogram pushed a commit that referenced this pull request Aug 30, 2022
Instead of changing the calls, replace the function bodies themselves.
This is useful for a number of reasons, see
#2920 for more information.

I have removed the math intrinsics tests because they are no longer
useful. Instead, I think `tinygo test math` should suffice.
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.

3 participants