You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
[CIR][Codegen] Fixes global variables that point to another globals (#1277)
Testing CIR with `csmith` is in progress! And this PR is not a small
one, so I need to apologize. But looks like there is a problem that may
affect the run time.
### Problem
Consider the next code:
```
#include <stdio.h>
typedef struct {
int f0 : 24;
int f1;
int f2;
} S;
static S g1 = {2799, 9, 123};
static int *g2 = &g1.f2;
int main() {
printf("check: %d\n",*g2);
return 0;
}
```
This program dumps anything but not `123`.
So basically we don't support global variables that refer to another
globals in the case of aggregate types.
This PR fixes global views for two cases: structs and array of structs
(see the tests).
There is an issue with unions too, but I think we will discuss it later,
once we will agree on the approach
### Some details
For the example above, `g1` variable is created in two steps:
1) The variable is created with the `S` type. In our case it's
`!cir.struct<struct "S" {!cir.array<!cir.int<u, 8> x 3>, !cir.int<s,
32>, !cir.int<s, 32>}`(which differs from the OG btw, and may be I need
to take a look at the bit fields again - but it's not a problem right
now)
2) For the left side we create anon structure of type `
!cir.struct<struct {!cir.int<u, 8>, !cir.int<u, 8>, !cir.int<u, 8>,
!cir.int<u, 8>, !cir.int<s, 32>, !cir.int<s, 32>}>` which is the same as
in OG and then `g1` is replaced with the new type.
Basically the same happens in the OG. But then the `replaceAllUsesWith`
solves all the possible problems.
In our case we can not do it this easy. The `g2` is:
```
cir.global @G2 = #cir.global_view<@g1, [2 : i32]> : !cir.ptr<!s32i>
```
The problem is in the indexes! After `g1` is replaced, the index in
`g2`'s `GlobalViewAttr`still points to the old type!!!
So we have to create a new `GlobalViewAttr` with new indexes!
### Solution
My solution is based on the `computeGlobalViewIndicesFromFlatOffset`
function from `CIRGenBuilder` that is basically a mapping from indexes
to offset. I compute an offset and then map it back to the indexes with
for the new type.
May be there is a better solution though, have some ideas?
#### Implementation details
Most of the changes are in the `CIRGenModule` where we do the
replacement.
Also, there are some changes in `CIRGenBuilder` - I moved
`computeGlobalViewIndicesFromFlatOffset` to `cpp` with no changes and
added the opposite function `computeOffsetFromGlobalViewIndices`.
One more fix is more important - is about `GlobalViewAttr` indexes
generation for vtable. I suggest we don't set the first index to zero in
CIR and add it in the lowering - in this case we can do it uniformly as
```
if (isa<mlir::LLVM::LLVMArrayType, mlir::LLVM::LLVMStructType>(sourceType))
indices.push_back(0);
```
The previous approach was not completely correct - we have to add the
leading zero index for the anon structures as well:
```
if (stTy.isIdentified())
indices.push_back(0); // Wrong
```
Thus, there are some changes in tests as well. It's not an unrelated
issue - we have to fix it - either now or as a separated PR that should
come before this one.
Also, I added tests with the covered cases with the original LLVM IR,
just for comparison - so there are some divergences as well.
Finally - this code was already tested with `csmith` and I don't have
any problems so far. Once we'll fix unions I will have more information.
0 commit comments