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

TODO: need to support opaque pointers #154

Closed
vtjnash opened this issue Feb 20, 2022 · 11 comments · Fixed by #172
Closed

TODO: need to support opaque pointers #154

vtjnash opened this issue Feb 20, 2022 · 11 comments · Fixed by #172
Assignees

Comments

@vtjnash
Copy link
Member

vtjnash commented Feb 20, 2022

Supporting opaque pointers will be a bit of a major design effort, and require a mini inference step to recover "best" types and tracking where to insert casts.

@hikari-no-yume
Copy link
Collaborator

This would be easier to do if we had an intermediate representation within the backend. It would be helpful for other things too (not producing redundant parentheses and other type casts for example).

@python3kgae
Copy link

We have PointerTypeAnalysis for DirectX backend to help recover the element type from opaque pointer.
Will it be useful here?
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/DirectX/PointerTypeAnalysis.cpp

@vtjnash
Copy link
Member Author

vtjnash commented Nov 5, 2022

alloca still has types, and otherwise void* is equivalent, so it likely shouldn't be necessary. But someone needs to put in the effort to handle it

@python3kgae
Copy link

Yes, void * should work for all cases.
Global variables have types too. But function parameter will need analysis to get pointer element type.

Off the topic, any plan for update to llvm 15?

@vtjnash
Copy link
Member Author

vtjnash commented Jul 13, 2023

In particular though, C defines cast to/from void* to be implicit (unlike C++), so we can likely just drop the types and use void instead, except when they are actually relevant to the operation (load, store, GEP, and alloca), just like opaque pointers in LLVM.

@hikari-no-yume
Copy link
Collaborator

hikari-no-yume commented Jul 13, 2023

We might need to be careful in which types we pick in order to avoid undefined behaviour issues with C's aliasing rules? I'm not sure…

Screenshot of paragraphs 6 and 7 of section 6.5 of C standard draft N1570

@vtjnash
Copy link
Member Author

vtjnash commented Jul 13, 2023

Yeah, to be extra pedantic, we should only ever use memcpy into a temporary buffer, instead of direct load and store on pointers. Currently we assume users will pass -fno-strict-aliasing to their compiler

@vtjnash
Copy link
Member Author

vtjnash commented Jul 13, 2023

Of note however, the intermediate types assigned to the pointee are of no relevance to that part of the C standard, only the type used for the load or store itself.

@hikari-no-yume
Copy link
Collaborator

Yeah I guess it's an existing issue and nothing is really changed by the switch to opaque types in LLVM.

@dpaoliello
Copy link
Collaborator

Of note however, the intermediate types assigned to the pointee are of no relevance to that part of the C standard, only the type used for the load or store itself.

I was just about to say this.

So far, the only place that I can't get to the actual type seems to be for return types and parameters in functions: I'm going to try using a modified version of the DirectX PointerTypeAnalysis to determine the types (the current analysis code loses the number of indirections, so I'll need to fork it to add in tracking for that) - unless folks are ok with all function signatures only using void* (which might be fine?).

@vtjnash
Copy link
Member Author

vtjnash commented Jul 13, 2023

I am happy with void* there. It is hardly the worst issue we have with function signatures (which would actually require a whole custom clang::TargetInfo to get the ABI mangling correct)

@dpaoliello dpaoliello self-assigned this Jul 13, 2023
This was referenced Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants