-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[llvm] [refactor] LLVMProgramImpl code clean up: part-2 #5187
[llvm] [refactor] LLVMProgramImpl code clean up: part-2 #5187
Conversation
✅ Deploy Preview for docsite-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I vaguely remember this function - the main motivation of get_llvm_program_impl
was a "temp" hack and we wanted to remove it in the future (as opposed to exposing a more backend agnostic version). Please correct me if I'm wrong! @k-ye
I guess we'll have to get rid of IMO, removing the backend-agnostic interface Ideally, we should move most of these members to "ProgramImpl", and make "Program" an interface class. In that way, lower-level objects such as CodeGenLLVM can keep "LlvmProgramImpl" instead of "Program". |
I agree that we should remove get_llvm_program_impl() first. One nit: inside codegen, instead of writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Related issue = #4800, #5114
Removed
Program::get_llvm_program_impl()
interface, so we dont have to expose backend-specific LlvmProgramImpl along with Program.