search

cdisselkoen / llvm-ir

LLVM IR in natural Rust data structures
MIT License
550 stars 46 forks source link

Type size v2 #62

Open joelimgu opened 7 months ago

joelimgu commented 7 months ago

This pull requests continues the work of @langston-barrett in #34 . I have taken into account the concerns you expressed in that pull request and rebased it to the current main branch.

I was not able to to address this concern: Can we have a comment pointing to the upstream function this is a port of, similar to how you did for getTypeSizeInBits? as I am not sure if this function is equivalent to the llvm function I have been able to identify (https://llvm.org/doxygen/DataLayout_8cpp_source.html#l00553).

And I would like feedback on how to deal with Type::TargetExtType as we don't have access to the type's data as explained in the enum.

I am no expert on llvm so any feedback is appreciated.

As a last note, I've seen the confusion between bits and bites multiple times in the code (I've hopefully fixed them), but upon successful validation of this code, a bit and byte types should be created moving forward.

Benjins commented 6 months ago

Not the maintainer, but wanted to note a few things:

  1. I tried pulling the code, and noticed that there are a couple compiler errors depending on which version of LLVM it uses. I left some more specific comments where they are
  2. For the upstream functions, they should all be in the DataLayout.h/DataLayout.cpp files. getTypeStoreSize and getTypeAllocSize for example
  3. I did notice that there are some discrepancies in the results between this ported version and the native LLVM version, however those appear to be from differences in how alignment is computed rather than the code in this PR. Specifically, nested structs and some vector types will give different answers in some cases. I'm not entirely sure what the issue is, but it'd probably be good to get some more test cases in either way
  4. For some reason CI didn't run? Github says it requires approval but I don't think it usually does. It builds w/ all the different LLVM versions, which catches things like missing version ranges
joelimgu commented 6 months ago

I've taken into account your comments, and added some tests, the CI is not working on the repo bc a maintainer needs to allow it but it runs on my repo for all llvm versions now. Let me know if you have any other remarks, I am happy to apply them!

If not we'll wait for the owner to respond and hopefully merge it if everything is ok! (I might need to clean the git history a bit I've made a mess).