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.

Sign up for GitHub

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

Jump to bottom

[NPM] Add hooks for llvm passes in llvmlite #1091

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[NPM] Add hooks for llvm passes in llvmlite #1091

wants to merge 7 commits into from

Conversation

yashssh
Copy link
Contributor

@yashssh yashssh commented Oct 24, 2024

  • Add hooks for most of the passes currently supported by the legacy pm
  • Add todo for future passes, simplify pass registration in newpassmanager.py
  • Add dot printer passes

@yashssh
Use C-macros to register passes and prevent duplication in c++ files
0ba654d 
Add todo for future passes, simplify pass registeration in newpassmanager.py

Add dot printer passes

Add hooks for most of the passes currently supported by legacy pm

Fix working of new APIs with llvm16
@yashssh yashssh mentioned this pull request Oct 25, 2024
Open
@gmarkall
Copy link
Member

gmarkall commented Nov 5, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link
Contributor

Azure Pipelines successfully started running 1 pipeline(s).

@gmarkall
Copy link
Member

gmarkall commented Nov 6, 2024

@yashssh Now that CI is up and working again, I think there is an issue with the Windows build that might be a real problem - do you know why it's not finding LLVMPY_module_AddVerifierPass?

The other issue appears to just be a C++ formatting issue.

gmarkall
gmarkall requested changes Nov 6, 2024
View reviewed changes
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

I just noticed, the problem on Windows is that many of the new functions are missing the API_EXPORT macro.

@yashssh
Copy link
Contributor Author

yashssh commented Nov 7, 2024

I'm not sure why windows is not able to find LLVMPY_module_AddVerifierPass while linux can, I have moved the definition out of the macro to see if it changes anything.

I just noticed, the problem on Windows is that many of the new functions are missing the API_EXPORT macro.

AFICT I have defined everything albeit not explicitly but using C++ macros to prevent rewriting same code over and over again. Can you point me to specific example if I have missed it?

@yashssh
Copy link
Contributor Author

yashssh commented Nov 7, 2024

It's indeed an issue with C++ macros not being expanded/evaluated correctly on windows as now it's failing with a different API LLVMPY_module_AddAAEvaluator.

@gmarkall
Copy link
Member

From out-of-band discussion: Graham to point to the usage of the API_EXPORT macro.

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
}

#define CGSCC_PASS(NAME) \
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
Copy link
Member

Choose a reason for hiding this comment

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

This is where I think API_EXPORT needs adding to enable these functions to be exported from the DLL in Windows:

Suggested change
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
API_EXPORT(void) LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
#include "PASSREGISTRY.def"

#define MODULE_PASS(NAME) \
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
API_EXPORT(void) LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
#include "PASSREGISTRY.def"

#define FUNCTION_PASS(NAME) \
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
API_EXPORT(void) LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
llvm::unwrap(MPM)->addPass(createModuleToFunctionPassAdaptor(NAME())); \
} \
void LLVMPY_function_Add##NAME(LLVMFunctionPassManagerRef FPM) { \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void LLVMPY_function_Add##NAME(LLVMFunctionPassManagerRef FPM) { \
API_EXPORT(void) LLVMPY_function_Add##NAME(LLVMFunctionPassManagerRef FPM) { \

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
#include "PASSREGISTRY.def"

#define LOOP_PASS(NAME) \
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \
API_EXPORT(void) LLVMPY_module_Add##NAME(LLVMModulePassManagerRef MPM) { \

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
llvm::unwrap(MPM)->addPass(createModuleToFunctionPassAdaptor( \
createFunctionToLoopPassAdaptor(NAME()))); \
} \
void LLVMPY_function_Add##NAME(LLVMFunctionPassManagerRef FPM) { \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void LLVMPY_function_Add##NAME(LLVMFunctionPassManagerRef FPM) { \
API_EXPORT(void) LLVMPY_function_Add##NAME(LLVMFunctionPassManagerRef FPM) { \

gmarkall
gmarkall reviewed Nov 19, 2024
View reviewed changes
ffi/newpassmanagers.cpp Outdated
}
#include "PASSREGISTRY.def"

const char *LLVMPY_getModuleLevelPasses() {
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs an API_EXPORT as well but I don't trust myself to get the syntax exactly right without trying it first, so no suggestion as such :-)

@gmarkall
Copy link
Member

@yashssh I added some notes on where I think adding API_EXPORT will resolve the Windows issues.

@yashssh
Copy link
Contributor Author

yashssh commented Nov 20, 2024

Thanks for the changes @gmarkall! I have updated the files, all tests seem to pass now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarkall gmarkall gmarkall requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
4 - Waiting on reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yashssh @gmarkall