[CIR][CodeGen][Bugfix] bitfields: use the storage type ifor the getMeberOp #261
Conversation
| Address CIRGenFunction::getAddrOfBitFieldStorage(LValue base, | ||
| const FieldDecl *field, | ||
| unsigned index, | ||
| unsigned size) { |
There was a problem hiding this comment.
Why are you changing the function signature? Why wasn't it getAddrOfBitFieldStorage in the first place?
There was a problem hiding this comment.
The function was introduced by me and in previous version it was not intended to work with bitfields only. After the last changes it will serve only them though. That's why I renamed it and added one more argument
| if (index == 0) | ||
| return base.getAddress(); | ||
|
|
||
| auto loc = getLoc(field->getLocation()); | ||
| auto fieldType = convertType(field->getType()); | ||
| auto fieldType = builder.getUIntNTy(size); |
There was a problem hiding this comment.
Please explain why it shouldn't use convertType here.
There was a problem hiding this comment.
convertType for the integer field is always 32, type mismatch guaranteed, see a longer comment below
There was a problem hiding this comment.
So please add assert(convertType(field->getType()) == ...)
| const unsigned SS = useVolatile ? info.VolatileStorageSize : info.StorageSize; | ||
| Address Addr = getAddrOfBitFieldStorage(base, field, Idx, SS); |
There was a problem hiding this comment.
What's LLVM codegen doing here? Does it call getAddrOfBitFieldStorage or getAddrOfField?
There was a problem hiding this comment.
there is no LLVM codegen here.
There was a problem hiding this comment.
I mean, we follow the principle of using the same skeleton as clang/lib/CodeGen, at least to the extend of what's possible.
I don't see anything similar to getAddrOfBitFieldStorage or getAddrOfField in clang/lib/CodeGen/*, and I want to understand what's the rationale there. Are those names introduced for specific functionality you refactor into helpers or are they complete new names for something that already existed in clang/lib/CodeGen? If so, I'm trying to understand why can't you just s/Emit/build and call it a day?
Please elaborate on a rationale |
|
@bcardosolopes So now, all the problems were investigated and everything works. The two problems appeared in the verification stage:
Answering one more time to your question about the Concerning indexes problems. Btw, for some reasons I thought that you have a sort of CI here - I saw some green check mark as if a PR was tested somehow. But after the second glance, there was nothing that was tested. Is it true? Once it sounds ok, I can push and updated PR here or create a new one, up to you. |
It happens.
Thanks for the detailed explanation, please add an assert with your assumption there then, in case for some ABI reasons something different show up, I'd prefer if it crashes at that point and we can easily fix.
Nice, I can see that. Thanks for catching this (and investigating a fix). Could be its own PR though, right? Sounds to me this isn't blocking a fix to land you bitfield work, but correct me if I'm wrong.
I wish we had, and I understand that green mark is misleading. I need to sort that out at some point! I currently do some internal testing on our codebase, but that's it.
If you can change the name of this PR than this one is fine, otherwise might be better to create a new one. |
Oh, this is #263, never mind :) |
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
|
Work here has been done in #268 |
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
…lvm#268) This is an updated PR for [PR llvm#233](llvm/clangir#233) with some fixes explained in [PR llvm#261](llvm/clangir#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
…lvm#268) This is an updated PR for [PR llvm#233](llvm#233) with some fixes explained in [PR llvm#261](llvm#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
…lvm#268) This is an updated PR for [PR llvm#233](llvm#233) with some fixes explained in [PR llvm#261](llvm#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
This is an updated PR for [PR #233](#233) with some fixes explained in [PR #261](#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
…lvm#268) This is an updated PR for [PR llvm#233](llvm/clangir#233) with some fixes explained in [PR llvm#261](llvm/clangir#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
…lvm#268) This is an updated PR for [PR llvm#233](llvm#233) with some fixes explained in [PR llvm#261](llvm#261) which now can be safely closed. First of all, let me introduce how do the bitfields looks like in CIR. For the struct `S` defined as following: ``` typedef struct { int a : 4; int b : 27; int c : 17; int d : 2; int e : 15; unsigned f; } S; ``` the CIR type is `!ty_22S22 = !cir.struct<struct "S" {!u32i, !u32i, !u16i, !u32i} #cir.record.decl.ast>` where all the bitfields are packed in the first three storages. Also, the next bugs was fixed: - type mismatch - index out of bounds - single bitfield of size < 8 The cases covered with tests.
Here is a fast-fix for the bit-fields PR, because now the tests don't pass. Basically, the bugs disclosed by the last changes in
GetMemberOp::verifyBypass get_member verifierThere is one bug that remain unfixed though - and it's kind of weird since everything was ok recently.
Speaking about the fix itself. Previously the result type for the
GetMemberOpwas inferred fromFieldDecl, now - from the storage size. Also, there are changes in tests: since storages for bitfields are unsigned, we don't need to add a cast after thegetMemverOp