[CIR][CIRGen] CIR generation for bitfields. Fixes #13

[gitoleg]

This PR introduces bitfelds support. I explicitly marked the PR as a draft, since I foresee some discussion here.
There are no tests so far - and I will add them once we agree on everything else (or won't do it if we disagree at all :) )

Probably, the key design decision is an extra pointer to CIRGenBitFieldInfo in the Value class. Unfortunately, it can not be done - at least from my point of view - in the same way, as it is done in the original CodeGen/CGValue.h. The rest of the code is a revision of the code generation in CodeGen .

Also, there are some changes due to clang-format application. Just noticed it(

Basically, the next toy example works.

#include <stdio.h>

typedef struct {
    int a1 : 4;
    int a2 : 28;
    int a3 : 16;
    int a4 : 3;
    int a5 : 17;
    int a6 : 25; 
} A;

void init(A* a) {
    a->a1 = 1;
    a->a2 = 321;
    a->a3 = 15;
    a->a4 = -2;
    a->a5 = -123;
    a->a6 = 1234;
}

void print(A* a) {
    printf("%d %d %d %d %d %d\n",
        a->a1,
        a->a2,
        a->a3,
        a->a4,
        a->a5,
        a->a6
    );
}

int main() {
    A a;
    init(&a);
    print(&a);
    return 0;
}

the output is:
1 321 15 -2 -123 1234

[bcardosolopes]

Yay, this is awesome! Thanks for putting up a PR, I'm gonna take a look tomorrow and get you some feedback!

[bcardosolopes]

The overall direction is good. The design I'm looking for here is a bit more higher level, but I'm fine with the more lower level as a start point. Next step (in a follow up PR) one could create more higher level abstractions to wrap some of the binop/shift details (for example, struct_element_addr could receive an additional bit offset argument), and the logic you add in this PR can then move to LoweringPrepare pass.

Did you had any thoughts on other higher level approaches?

This PR introduces bitfelds support. I explicitly marked the PR as a draft, since I foresee some discussion here. There are no tests so far - and I will add them once we agree on everything else (or won't do it if we disagree at all :) )

I encourage you to add tests as early as possible because it makes it easier for reviews (we can look at the generated code to think about whether things make sense). I'm sure we can always collaborate on a direction that will work for everyone :)

Probably, the key design decision is an extra pointer to CIRGenBitFieldInfo in the Value class. Unfortunately, it can not be done - at least from my point of view - in the same way, as it is done in the original CodeGen/CGValue.h. The rest of the code is a revision of the code generation in CodeGen .

The overall approach is good, one general principle is that we don't add support for paths that we don't write tests for, so the lack of tests makes it harder to judge whether some of it also makes sense (feel free to add asserts and start simple).

Also, there are some changes due to clang-format application. Just noticed it(

It's easier when that it's done already in the first PR, because if patch is already good we just need to merge it.

Basically, the next toy example works.

Doesn't build for me, when I try to build a more reduced version of the example from the description, it fails with Assertion failed: (Dst.isSimple() && "only implemented simple"), also another good use of a testcase in the PR.

Seems like this is leading to several failures (ninja check-clang-cir):

Failed Tests (11):
  Clang :: CIR/CodeGen/basic.cpp
  Clang :: CIR/CodeGen/binop.cpp
  Clang :: CIR/CodeGen/cast.cpp
  Clang :: CIR/CodeGen/derived-to-base.cpp
  Clang :: CIR/CodeGen/loop.cpp
  Clang :: CIR/CodeGen/types.c
  Clang :: CIR/Transforms/lifetime-check-agg.cpp
  Clang :: CIR/Transforms/lifetime-check-remarks.cpp
  Clang :: CIR/Transforms/lifetime-check.cpp
  Clang :: CIR/Transforms/lifetime-loop-valid.cpp
  Clang :: CIR/Transforms/lifetime-loop.cpp

[gitoleg]

@bcardosolopes

So the plan is the following: I add tests, maybe refactor the code a little, and then will cal you for the next review.

Did you had any thoughts on other higher level approaches?

Not really. As I said, I took the code in clang/lib/Codegen/ as an example.

Doesn't build for me, when I try to build a more reduced version of the example from the description, it fails with Assertion failed: (Dst.isSimple() && "only implemented simple"), also another good use of a testcase in the PR.

that's strange, nothing similar on my side. Will try to reproduce.

Seems like this is leading to several failures (ninja check-clang-cir):

Well, that's bad( there is an idea though why did this happen

[gitoleg]

@bcardosolopes

Doesn't build for me, when I try to build a more reduced version of the example from the description, it fails with Assertion failed: (Dst.isSimple() && "only implemented simple"), also another good use of a testcase in the PR.

Reproduced. If the code is in in .cpp file. For .c everything is fine. Interesting.

[sitio-couto]

@gitoleg pretty neat!

I just had some minor suggestions and questions.

[bcardosolopes]

So the plan is the following: I add tests, maybe refactor the code a little, and then will cal you for the next review.

Sounds good!

Reproduced. If the code is in in .cpp file. For .c everything is fine. Interesting.

Gotcha, please add testcases both for c and c++ then!

[gitoleg]

@bcardosolopes @sitio-couto
I updated the PR and looks like now it is readable say the least.

I addressed almost every issue, but still we lack of tests. The idea for you is to take a look at the code and tests, and may be add more ideas for the latter.
Also, I removed some code that doesn't relate to the topic and will be added as an additional PR.

Speaking about the tests were failing - there is a function hasBooleanRepresentation that kind of ruin some of the tests now (since I created one more call from CIRGenExprScalar to the buildLoadOfLValue). The question is - why do we need this check at all? Boolean values have the same representation both in memory and after load - cir.load and I don't see why we check something. It makes sense in the original Codegen, since llvm IR has i1 and i8 in depends on what we're doing.

[bcardosolopes]

@bcardosolopes @sitio-couto I updated the PR and looks like now it is readable say the least.
I addressed almost every issue, but still we lack of tests.

Thanks for addressing the issues and for your patience! Can you also change the tests to use substitution blocks? There are other examples in the test directories. It's currently not easy to spot the relationship of the generated CIR with the source code in the tests.

The idea for you is to take a look at the code and tests, and may be add more ideas for the latter. Also, I removed some code that doesn't relate to the topic and will be added as an additional PR.

Sweet! Can you rebase? @sitio-couto landed bunch of changes to structs. Do all tests pass? I'm gonna give it a try then.

I've had second thoughts, and since you are here this might be a good opportunity to introduce a higher level operation already, by embed more information in cir.get_member, and move the code you just added to clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp instead, where the checks are going to be CIR -> CIR. Full disclosure: my change of thought here comes from the fact that these might be hard to bring back later, and the amount of shift and other opts are going to unnecessary compromise the readability on the highest level CIR.

Speaking about the tests were failing - there is a function hasBooleanRepresentation that kind of ruin some of the tests now (since I created one more call from CIRGenExprScalar to the buildLoadOfLValue). The question is - why do we need this check at all? Boolean values have the same representation both in memory and after load - cir.load and I don't see why we check something. It makes sense in the original Codegen, since llvm IR has i1 and i8 in depends on what we're doing.

You're talking about the call from buildFromMemory, right? The assert is there because we were lacking a testcase for this path, but you are in principle right. I believe you can trigger this path without this PR, so how about you remove the unreachable and add a testcase in a dependency PR for this one? This would be the best approach!

[bcardosolopes]

Some inline comments too!

[bcardosolopes]

Also, as you fix things, can you mark relevant comments as resolved? That helps the reviewers a bunch, thanks!

[gitoleg]

@bcardosolopes
So, speaking about embedding some additional information into the GetMemberOp.

The first solution is to add at least a couple of integer attributes to the operations that will define the bitfield: an offset in the storage and the size. The problem is that load from and store to bitfield differ one from another (have different shift operations). So when we will pre-lower the GetMemberOp, we will also need to know what kind of op is in question. Hence, this info should be also added to the GetMemberOp. The problem with the result type (that should be integer type of bitfield size but not of the storage size) is more technical one and can be solved. As a result, this is doable, but I still have some doubts, if we want to go this way.

Another solution is to introduce two new operations, for example GetBitfeildOp and SetBitfeidOp or similar and use them instead of GemMemberOp. This is also may also contain some hidden caveats but looks more attractive for me. Then, in the LoweringPrepare we may replace this operations with GemMemberOp and shifts, and-s, or-s and so on.

So, what do you think?

[sitio-couto]

@bcardosolopes @gitoleg a few of my thoughts on the discussion:

I've had second thoughts, and since you are here this might be a good opportunity to introduce a higher-level operation already

Sounds like a lot of work for a single PR. Maybe we could break this into 3 separate PRs: this one, the creation and lowering of the higher-level ops, and updating the codegen to favor higher-level ops. There might be some wasted work in-between these PRs, but changing the current PR to use higher-level ops will likely be just as wasteful.

The problem is that load from and store to bitfield differ one from another (have different shift operations). So when we will pre-lower the GetMemberOp, we will also need to know what kind of op is in question.

The idea of GetMemberOp is to have a generic way to access record members. Having custom attributes for specific types of records would go against its design. Ideally, how a member is accessed by GetMemberOp should be inferred from the record/member type. If we choose this route, I would suggest that the extra info is added to the cir.struct_type instead of the GetMemberOp since bitfields are only valid within records.

Another solution is to introduce two new operations, for example GetBitfeildOp and SetBitfeidOp or similar and use them instead of GemMemberOp.

I would suggest we start like this as well. There will likely be a large intersection between these and GetMemberOp, but starting separately should make it clearer how to merge them later.

[bcardosolopes]

@gitoleg @sitio-couto thanks for the feedback.

You're both right, cir.get_member doesn't indeed sound like the best place to land this for now. I like the idea of splitting this into multiple PRs and make it incremental, it will be easier to iterate on something like GetBitfeildOp and SetBitfeidOp after we land this.

[bcardosolopes]

Marking this as needing changes because rebase and latest comments, but this almost ready to ship per latest discussion - raising representation will come into followup PRs. Thanks again for working on this @gitoleg