support larger bit field #528
Conversation
temporary disable other test using test specific int bits use offset for base add back tests
chengchingwen
force-pushed
the
master
branch
from
1effd39 to
c31d785
Compare
|
@christiangnrd Could you help review this PR? |
|
I'm not familiar enough with this part of the codebase to review this. I gave it a try but it would take more bandwidth than I have at the moment. |
| ex = :(f === $(QuoteNode(fsym)) && return (Ptr{$ty}(x + $(4d)), $r, $w)) | ||
| iszero(w) && continue | ||
| d, r = divrem(offset, 8) | ||
| @assert r + w <= 128 "Cannot handle bitfield larger than 128 bits" |
There was a problem hiding this comment.
Is this documented in the standard?
There was a problem hiding this comment.
I don't think so. It's just super rare for a c/c++ code to have a primitive type that is larger than 128 bits, so I thought it would be reasonable to be left unhandled until we find a real use case.
There was a problem hiding this comment.
The test only has 64-bit test cases. Do you have a specific need for the 128-bit support?
There was a problem hiding this comment.
No, I only need 64-bit support
| struct LargeBitField { | ||
| double a; | ||
| short b; | ||
| unsigned long long c : 16; |
There was a problem hiding this comment.
@chengchingwen It looks like you removed the logic for handling straddle. Does it work if you change this to 20?
unsigned long long c : 16;
There was a problem hiding this comment.
20+48>64, so d overlaps another "word".
There was a problem hiding this comment.
@Gnimuc The reason that the old logic has a branch for straddle is because they only perform 32-bit load on 32-bit-aligned "offset" thus a bit field would span across two 32-bit segments as long as the offset is smaller than multiples of 32 and adding the bit width exceeds multiples of 32. The new logic performs variable-sized load on 8-bit-aligned "offset" based on the required bit size.
The bitfield "offset" obtained by getOffsetOf is not the start point of the whole bitfield but a point determined by the compiler that could be a middle point of that bitfield. Having 20 + 48 wouldn't necessarily cause d to overlap another "word" because of alignment and padding. Meanwhile, the "offset" obtained might just exceed the 20 bit part.
There was a problem hiding this comment.
@chengchingwen It looks like the old logic is for supporting the AIX layout.
I think we can add an option to let users select which layout they need.
The bitfield "offset" obtained by getOffsetOf is not the start point of the whole bitfield but a point determined by the compiler that could be a middle point of that bitfield. Having 20 + 48 wouldn't necessarily cause d to overlap another "word" because of alignment and padding. Meanwhile, the "offset" obtained might just exceed the 20 bit part.
As we use the value from Clang, could you give it check whether your current impl is compatible with Clang? https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/RecordLayoutBuilder.cpp#L1534
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (4)
- src/generator/codegen.jl: Language not supported
- test/bitfield/bitfield.c: Language not supported
- test/bitfield/bitfield.h: Language not supported
- test/test_bitfield.jl: Language not supported
This PR reworks the bit field get/set property code to support bit field larger than 32 bits