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

Standardize LLVM module verification #46387

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Standardize LLVM module verification #46387

merged 3 commits into from
Oct 24, 2022

Conversation

pchintalapudi
Copy link
Member

Previously we verified LLVM modules in some optimization passes, which have already caught a number of llvmcall errors previously. This PR just standardizes their location to be around optimization pipeline call points. We do lose some coverage in that if a pass does something bad but a later pass fixes up the offending code it won't get called out, but that behavior can be recovered with CPPFLAGS=-DJL_VERIFY_PASSES.

@pchintalapudi pchintalapudi added the compiler:llvm For issues that relate to LLVM label Aug 18, 2022
@pchintalapudi pchintalapudi force-pushed the pc/verify-module branch 2 times, most recently from e308ade to 2c05eb2 Compare August 19, 2022 04:30
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 2, 2022

Can you make this JL_VERIFY_PASSES tied to JL_NDEBUG (by default), so that it could be set separately, but doesn't have to be?

@pchintalapudi
Copy link
Member Author

It would appear that our LateLowerGC pass is doing something bad with phi nodes

This was referenced Sep 7, 2022
Merged
Closed
@oscardssmith
Copy link
Member

Any chance #44463 was buggy?

@pchintalapudi
Copy link
Member Author

Any chance #44463 was buggy?

No, the issue was solved in #46662 (improper handling on phi nodes from switches).

@vchuravy vchuravy merged commit fe72f81 into master Oct 24, 2022
@vchuravy vchuravy deleted the pc/verify-module branch October 24, 2022 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vchuravy vchuravy vchuravy approved these changes

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees
No one assigned
Labels
compiler:llvm For issues that relate to LLVM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pchintalapudi @vtjnash @oscardssmith @vchuravy