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

codegen: implement isdefined_tfunc check more similarly to inference #52069

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Nov 7, 2023

No description provided.

@vtjnash vtjnash requested review from gbaraldi and vchuravy November 7, 2023 18:29
@vtjnash vtjnash added the compiler:codegen Generation of LLVM IR and native code label Nov 7, 2023
@gbaraldi
Copy link
Member

gbaraldi commented Nov 7, 2023

I guess this can supercede #52068?

@vtjnash vtjnash marked this pull request as draft November 7, 2023 19:06
@vtjnash
Copy link
Member Author

vtjnash commented Nov 7, 2023

Oops, turns out that this optimization is unsound for inference / codegen to attempt since some packages (e.g. NetworkOptions) relies on the non-existence of this optimization--we therefore may need to delete this optimization from inference.

@oscardssmith
Copy link
Member

some packages (e.g. NetworkOptions) relies on the non-existence of this optimization

How can they tell whether this optimization was performed?

@vtjnash
Copy link
Member Author

vtjnash commented Nov 10, 2023

They segfault if the code gets reinferred

@vtjnash
Copy link
Member Author

vtjnash commented Apr 16, 2024

Part of the cleanup here is perhaps worthwhile (merging code paths into a common helper function), but the main change has been removed from inference for correctness reasons in #53750

@vtjnash vtjnash changed the title codegen: optimize isdefined check for loading fields like inference codegen: implement isdefined_tfunc check similarly to inference Jun 4, 2024
@vtjnash vtjnash changed the title codegen: implement isdefined_tfunc check similarly to inference codegen: implement isdefined_tfunc check more similarly to inference Jun 4, 2024
@vtjnash vtjnash force-pushed the jn/undeffield-codegen-opt branch from 1641188 to daf57cc Compare June 4, 2024 13:49
@vtjnash vtjnash marked this pull request as ready for review June 4, 2024 13:49
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jun 4, 2024
@vtjnash vtjnash force-pushed the jn/undeffield-codegen-opt branch from daf57cc to 91a74dc Compare June 4, 2024 19:35
@vchuravy vchuravy removed their request for review June 4, 2024 22:33
@DilumAluthge
Copy link
Member

CI is all green here. @vtjnash Is this good to merge now, or are you still waiting on a review from Valentin and/or Gabriel?

@vtjnash
Copy link
Member Author

vtjnash commented Jun 5, 2024

It is fine now to merge

@DilumAluthge DilumAluthge merged commit 24acc68 into master Jun 5, 2024
7 checks passed
@DilumAluthge DilumAluthge deleted the jn/undeffield-codegen-opt branch June 5, 2024 03:10
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:codegen Generation of LLVM IR and native code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants