[CIR][CodeGen] Bitfield operations #279

gitoleg · 2023-10-11T11:44:42Z

As we discussed in #233, there is a desire to have CIR operations for bit fields set/get access and do all the stuff in the LoweringPrepare.

There is one thing I want to discuss, that's why the PR is marked as a draft now.
Looks like I have to introduce some redundant helpers for all these or and shift operations: while we were in the CodeGen area, we used CIRGenBuilder and we could easily extend it. I bet we don't want to depend from CodeGen in the LoweringPrepare. Once it's true. what is a good place for all this common things? As an idea, we could introduce one more layer for builder, with no state involved - just helpers and nothing else. But again, what is a good place for it from your point of view?

clang/include/clang/CIR/Dialect/IR/CIROps.td

clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp

bcardosolopes · 2023-10-18T01:19:29Z

There is one thing I want to discuss, that's why the PR is marked as a draft now.
Looks like I have to introduce some redundant helpers for all these or and shift operations: while we were in the CodeGen area, we used CIRGenBuilder and we could easily extend it. I bet we don't want to depend from CodeGen in the LoweringPrepare. Once it's true. what is a good place for all this common things? As an idea, we could introduce one more layer for builder, with no state involved - just helpers and nothing else. But again, what is a good place for it from your point of view?

This is a good idea, maybe something like Dialect/Builder? Any suggestion? Another possible solution is to put these things in clang/include/clang/CIR/Dialect/IR/CIRDialect.h for now and if it keeps growing you could move to another directory. Maybe Dialect/Builder would be best? Ideally if the codegen builder can end up reusing the same component that is also great.

gitoleg · 2023-10-20T08:49:17Z

@bcardosolopes Summary, what's done

most of the helpers from the CIRGenBuilder moved to the CIRBaseBuilder, the latter is a parent of the former now. The CIRBaseBuilder has no state, hence the helpers there.
added expamples to the description of the operations. Don't know if it's really necessary, since the bitfield operations are erased on the LoweringPrepare stage

bcardosolopes

Thanks for putting more effort on this.

Can you move the changes for CIRGenBuilder.h and helpers that aren't related with the added instructions to another pre-requisite PR? Should be an easy merge.
Can you add start adding testcases? Hard to look into PRs without testcases to play with.

Cheers!

clang/include/clang/CIR/Dialect/IR/CIROps.td

bcardosolopes · 2023-10-23T01:44:49Z

2. added expamples to the description of the operations. Don't know if it's really necessary, since the bitfield operations are erased on the LoweringPrepare stage

Definitely useful for analysis paths before lowering prepare though!

gitoleg · 2023-10-23T14:05:03Z

Can you add start adding testcases? Hard to look into PRs without testcases to play with.

well ... Is it possible to write tests that will work with cir before LoweringPrepare pass?
Otherwise, once the current bitfield tests work - we may think it's enough

bcardosolopes · 2023-10-25T23:22:19Z

well ... Is it possible to write tests that will work with cir before LoweringPrepare pass?

Yes, plain -emit-cir will give you that. LoweringPrepare only really runs before -emit-llvm.

bcardosolopes · 2023-10-25T23:22:54Z

Can you please rebase? @lanza applied one rebase against upstream round, and github is now confused!

gitoleg · 2023-10-31T12:03:26Z

@bcardosolopes

Yes, plain -emit-cir will give you that. LoweringPrepare only really runs before -emit-llvm.

well, it could be a chance that something is wrong on my side, but I don't see any of these operations with -emit-cir. More than, old bitfields tests work with no changes from my side, i.e. with expectation of all these shifts, or-s and etc. Looks like loweringPrepare works in any case.

Speaking about custom asm - I can suggest to use just s-exp, as they are good for this amount of information I have to dump. Otherwise, I can fix it, and use any other separator on your taste.

So far I will prepare a separate PR for the builder.

As discussed in #279, we split `CIRGenBuilder` in two parts, which make some of the helpers usable outside of the `CodeGen` part. Basically, I placed casts and binary operations into a separate class, `CIRBaseBuilder`. Later, it can be extended with another helpers. But right now an idea to have a state less builder as a base one.

bcardosolopes · 2023-11-02T00:54:15Z

well, it could be a chance that something is wrong on my side, but I don't see any of these operations with -emit-cir. More than, old bitfields tests work with no changes from my side, i.e. with expectation of all these shifts, or-s and etc. Looks like loweringPrepare works in any case.

We need tests that exercise what gets created by createSetBitfield and others. There's an example in clang/test/CIR/CodeGen/static.cpp.

bcardosolopes

Thanks for the fixes, one more round of reviews!

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

gitoleg · 2023-11-02T14:00:14Z

@bcardosolopes
ready!

Also, please add an alias for it in AliasResult getAlias(Attribute attr, raw_ostream &os) in CIRDialect.cpp so that operations can refer to an easy #bitfield_fileXYZ

Well, I'm not sure I did it right, I just dump the name as an alias for the whole bitfield info

Also, the next thought came to my mind: there is kind of redundancy or even something that is not really clean or clear. Look, we first emit get_member in order to have an access to a storage where a bitfield resides, and then call get_bitfield or set_bitfield:

%3 = cir.get_member %2[1] {name = "d"} : !cir.ptr<!ty_22S22> -> !cir.ptr<!u32i>
%4 = cir.get_bitfield %3(#d) : !cir.ptr<!u32i> -> !s32i

Note, that in the get_member call we see the name of the bitfield, though it's not true - we just want to access a storage by index 1.
I would try to emit get_member in the LoweringPrepare as well, i.e. leave only get/set bitfield in the code before this pass. What do you think? if it's ok - I will prepare a separate PR once this one will be merged.

bcardosolopes · 2023-11-03T00:51:10Z

Note, that in the get_member call we see the name of the bitfield, though it's not true - we just want to access a storage by index 1.

The name is not really necessary, but we like to keep it around for making CIR a bit easier to read. Might be worth removing in the future, but should be good for now.

I would try to emit get_member in the LoweringPrepare as well, i.e. leave only get/set bitfield in the code before this pass. What do you think? if it's ok - I will prepare a separate PR once this one will be merged.

I can see where you come from, but turns out it's important that the get_member is explicit because we can always track the uses of structs by looking at those ops. If we instead embed it in {get,set}bitfield we now have to track one more operation for detecting member access in every analysis we could write.

bcardosolopes

This is awesome, almost there. Thanks for adding the testcases and improving docs.

clang/include/clang/CIR/Dialect/IR/CIROps.td

clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp

clang/include/clang/CIR/Dialect/IR/CIROps.td

gitoleg · 2023-11-07T07:58:24Z

@bcardosolopes
one more round of changes is ready)

bcardosolopes

Awesome, one last bit and this is ready to land.

clang/include/clang/CIR/Dialect/IR/CIROps.td

bcardosolopes

One more comment on printing and needs rebasing!

clang/include/clang/CIR/Dialect/IR/CIROps.td

clang/test/CIR/CodeGen/bitfield-ops.c

gitoleg · 2023-11-30T11:40:00Z

@bcardosolopes don't forget about this one)

bcardosolopes

Thanks for your patience, good to go!

As discussed in #279, we split `CIRGenBuilder` in two parts, which make some of the helpers usable outside of the `CodeGen` part. Basically, I placed casts and binary operations into a separate class, `CIRBaseBuilder`. Later, it can be extended with another helpers. But right now an idea to have a state less builder as a base one.

As we discussed in #233, there is a desire to have CIR operations for bit fields set/get access and do all the stuff in the `LoweringPrepare`. There is one thing I want to discuss, that's why the PR is marked as a draft now. Looks like I have to introduce some redundant helpers for all these `or` and `shift` operations: while we were in the `CodeGen` area, we used `CIRGenBuilder` and we could easily extend it. I bet we don't want to depend from `CodeGen` in the `LoweringPrepare`. Once it's true. what is a good place for all this common things? As an idea, we could introduce one more layer for builder, with no state involved - just helpers and nothing else. But again, what is a good place for it from your point of view?

As discussed in #279, we split `CIRGenBuilder` in two parts, which make some of the helpers usable outside of the `CodeGen` part. Basically, I placed casts and binary operations into a separate class, `CIRBaseBuilder`. Later, it can be extended with another helpers. But right now an idea to have a state less builder as a base one.

As we discussed in #233, there is a desire to have CIR operations for bit fields set/get access and do all the stuff in the `LoweringPrepare`. There is one thing I want to discuss, that's why the PR is marked as a draft now. Looks like I have to introduce some redundant helpers for all these `or` and `shift` operations: while we were in the `CodeGen` area, we used `CIRGenBuilder` and we could easily extend it. I bet we don't want to depend from `CodeGen` in the `LoweringPrepare`. Once it's true. what is a good place for all this common things? As an idea, we could introduce one more layer for builder, with no state involved - just helpers and nothing else. But again, what is a good place for it from your point of view?

As discussed in #279, we split `CIRGenBuilder` in two parts, which make some of the helpers usable outside of the `CodeGen` part. Basically, I placed casts and binary operations into a separate class, `CIRBaseBuilder`. Later, it can be extended with another helpers. But right now an idea to have a state less builder as a base one.

As we discussed in #233, there is a desire to have CIR operations for bit fields set/get access and do all the stuff in the `LoweringPrepare`. There is one thing I want to discuss, that's why the PR is marked as a draft now. Looks like I have to introduce some redundant helpers for all these `or` and `shift` operations: while we were in the `CodeGen` area, we used `CIRGenBuilder` and we could easily extend it. I bet we don't want to depend from `CodeGen` in the `LoweringPrepare`. Once it's true. what is a good place for all this common things? As an idea, we could introduce one more layer for builder, with no state involved - just helpers and nothing else. But again, what is a good place for it from your point of view?

As discussed in llvm#279, we split `CIRGenBuilder` in two parts, which make some of the helpers usable outside of the `CodeGen` part. Basically, I placed casts and binary operations into a separate class, `CIRBaseBuilder`. Later, it can be extended with another helpers. But right now an idea to have a state less builder as a base one.

As we discussed in llvm#233, there is a desire to have CIR operations for bit fields set/get access and do all the stuff in the `LoweringPrepare`. There is one thing I want to discuss, that's why the PR is marked as a draft now. Looks like I have to introduce some redundant helpers for all these `or` and `shift` operations: while we were in the `CodeGen` area, we used `CIRGenBuilder` and we could easily extend it. I bet we don't want to depend from `CodeGen` in the `LoweringPrepare`. Once it's true. what is a good place for all this common things? As an idea, we could introduce one more layer for builder, with no state involved - just helpers and nothing else. But again, what is a good place for it from your point of view?