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

Add test that validates swt clone module function references. #1477

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Pavel-Durov
Copy link
Contributor

@Pavel-Durov Pavel-Durov commented Nov 25, 2024

Add test to validate ykjit/ykllvm#216.

@Pavel-Durov Pavel-Durov force-pushed the fix-function-reference-yk-module-clone-pass branch from ce4bf45 to f1d58ed Compare December 7, 2024 14:10
@Pavel-Durov
Copy link
Contributor Author

updated ykllvm submodule with the latest ykllvm version since ykjit/ykllvm#216 was merged.

#include <yk.h>
#include <yk_testing.h>

int add(int i, int j) { return i + j; }
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be __attribute__((yk_outline)): at the moment I would expect that these are inlined, so the test will pass "by accident" (TM). Also, the stderr lang_test should check that the IR actually looks like we expect. Specifically, we should remove jit-pre-opt from YKD_LOG_IR but we should actually check that the functions, with the names we expect, are present in the AOT IR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will update.
But there's a problem with adding IR checks here because we don't serialize the cloned/optimized functions at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine: if the AOT IR should only refer to the unoptimised functions, then that's our test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated 👉 6bf7c04 and 👉 0dedb05

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a call dec in the trace IR too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I get the trace IR?

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends which you want: aot, jit-pre-opt, or jit-post-opt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, got it.
I got confused cause originally we said to remove jit-pre-opt from YKD_LOG_IR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should there be a call dec in the trace IR too?

Added trace checks 👉 d4b95bf

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check the call is to dec? I'm not totally sure off-hand if that is possible or not.

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