Skip to content

Conversation

dwightguth
Copy link
Collaborator

Previously we still had some references to typed pointers in .ll files and in API calls that still created typed pointers when working with older versions of LLVM. This led to some invalid bitcode being generated in ways that wasn't necessarily a problem, but which caused problems when I was making changes to that code. This PR is a preliminary to further changes which removes all remaining references to the API constructor for typed pointers and removes all typed pointers from flat bitcode files.

Dwight Guth added 4 commits July 8, 2024 16:41
On llvm 15, the llvm::PointerType::getUnqual(llvm::Type *) function
still creates typed pointers. We don't want to use typed pointers ata
ll, so we remove these call sites and replace them with the getUnqual
overload that uses opaque pointers.

Some further refactoring to condense and clean up the code is probably a
good idea in the future; this makes a lot of the code kind of weird
because I did a purely mechanical translation rather than attempting to
make the code look the way it would if we had written it for the first
time with opaque pointers.
On llvm 15, CreateMalloc seems to be broken in some cases when working
with opaque pointers. This is probably due to an llvm bug. However, this
function has a lot of complexity when really it's doing something pretty
simple. So I have replaced the complex version of the code with a fairly
simple version that just creates the desired CallInst directly.
@rv-jenkins rv-jenkins changed the base branch from master to develop July 8, 2024 21:57
@dwightguth dwightguth marked this pull request as ready for review July 9, 2024 00:51
Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

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

I made some comments to improve the code quality. Sorry for being pedantic.
Also, runtime/main/main.ll still contains typed pointers, can this be a problem in the future?

@dwightguth
Copy link
Collaborator Author

Don't worry at all; your suggestions do all make the code much nicer; I don't mind doing it and appreciate the thorough review. I believe I have now addressed all the issues. I will mark you for re-review as soon as I confirm that the tests are still passing.

Copy link
Collaborator

@theo25 theo25 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Collaborator

@Robertorosmaninho Robertorosmaninho left a comment

Choose a reason for hiding this comment

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

Thank you for addressing all comments! The code looks much nicer indeed!
LGTM!

@rv-jenkins rv-jenkins merged commit ac9cff8 into develop Jul 9, 2024
@rv-jenkins rv-jenkins deleted the opaque branch July 9, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants