Skip to content

transform: work around renamed return type after merging LLVM modules #1898

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
Merged
merged 1 commit into from
Oct 28, 2021
Merged

transform: work around renamed return type after merging LLVM modules #1898

merged 1 commit into from
Oct 28, 2021

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented May 20, 2021 •
edited
Loading

This fix is very similar to #1768, but now for the return type. It fixes the issue in #1887.

Like #1768, I'm not sure how to test this as it is very specific to certain renames that LLVM does and that don't seem very reproducable.

@aykevl
transform: work around renamed return type after merging LLVM modules
7da1a06 
This fix is very similar to
#1768, but now for the return
type. It fixes the issue in
#1887.

Like #1768, I'm not sure how to test this as it is very specific to
certain renames that LLVM does and that don't seem very reproducable.
@aykevl aykevl mentioned this pull request May 20, 2021
Closed
@deadprogram
Copy link
Member

I do not understand the implications of this change sufficiently, even though it is just one line. Are we sure this is safe?

@aykevl aykevl mentioned this pull request Oct 21, 2021
Closed
@aykevl
Copy link
Member Author

aykevl commented Oct 21, 2021

Yes, I'm pretty sure it is :)
Also, this continues to be a problem (see #1887 (comment) and #2197).

@aykevl aykevl requested a review from deadprogram October 27, 2021 13:19
niaow
niaow requested changes Oct 27, 2021
View reviewed changes
Copy link
Member

@niaow niaow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this resolves the problem with this specific bug, although it looks like we may have another similar bug in another pass. Can we add a test to the transform first (put something into the testdata/interface.ll)?

niaow
niaow approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

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

Nevermind, I actually see what you mean now. Sorry. LGTM.

@deadprogram
Copy link
Member

Thanks @aykevl for the work-around and @niaow for reviewing. Now merging.

@deadprogram deadprogram merged commit f99c600 into dev Oct 28, 2021
@deadprogram deadprogram deleted the fix-issue1887 branch October 28, 2021 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niaow niaow niaow approved these changes

@deadprogram deadprogram Awaiting requested review from deadprogram

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aykevl @deadprogram @niaow