[CIR] [workaround] Don't use InactiveUnionFieldAttr for non-largest member

[ChuanqiXu9]

Close https://github.com/llvm/clangir/issues/1131

I took more time than I thought on this. The problem seems to be more complex than I thought. And the patch itself is a workaround to stop the regression.

---

Following off are some details that we can look back future:

1. The direct problem is, when we had paddings for the initializers for a union, with the use of InactiveUnionFieldAttr, the line to compare the synthesized type and desired type (the union type) fails. https://github.com/llvm/clangir/blob/3aed38cf52e72cb51a907fad9dd53802f6505b81/clang/lib/CIR/CodeGen/CIRGenExprConst.cpp#L456-L458

then the type of the initializer is not considered to be an union, so the computed size is not correct, which is the reason for the crash.

2. It is worth to note that, before we introduce InactiveUnionFieldAttr, the comparison between the synthesized type and the desired type would fail too. But we can have the right size after all. So the reproducer in https://github.com/llvm/clangir/issues/1131 works.

3. It doesn't work if we don't add these padding bits if we used InactiveUnionFieldAttr. It can avoid the crash. But then when lowering the code, we have to adding these padding bits by our selves. The problem is the type mismatch.

In such manner, in https://github.com/llvm/clangir/blob/3aed38cf52e72cb51a907fad9dd53802f6505b81/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp#L433, the generated llvm type for the initializer of the union is struct { i64 }. But the generated type for the init value (in https://github.com/llvm/clangir/blob/3aed38cf52e72cb51a907fad9dd53802f6505b81/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp#L447) is i32. Then we have to consider how to instantiate a struct { i64 } with an i32. The specific example is easy by using zext instruction. But how about the generic case? Especially the type of the struct can be not compatible with the init type. This may not be super hard but I'd like to not fix it in the patch.

4. What's the problematic case now? It is similar to https://github.com/llvm/clangir/pull/1007 but the size of types in the union are not the same and we tried to init the union with the smaller types. Then we failed to recognize them as the same type so that we will failed to recognize their context as an array.

[bruteforceboy]

Hi! Thanks for looking into this again. For the case below the current implementation still fails to lower to LLVM, but works fine without InactiveUnionFieldAttr

typedef struct {
  int a, b, c, d;
} T;

typedef union {
  struct {
    int a;
    long b;
  };
  T c;
} S;

S s = {.c = {1, 2, 3, 4}};

Is this expected?

[bcardosolopes]

then the type of the initializer is not considered to be an union, so the computed size is not correct, which is the reason for the crash.

Have you considered making isLayoutIdentical more robust? It's possible we might need to use layoutInfo for taking some of the decisions (it's lazily computed and can be augmented to do more work), etc. It's probably best if we can hide this extra logic and keep some more similarity to OG skeleton to the extend we can.

[ChuanqiXu9]

Hi! Thanks for looking into this again. For the case below the current implementation still fails to lower to LLVM, but works fine without InactiveUnionFieldAttr

typedef struct {
  int a, b, c, d;
} T;

typedef union {
  struct {
    int a;
    long b;
  };
  T c;
} S;

S s = {.c = {1, 2, 3, 4}};

Is this expected?

Yes, this is more or less "expected". I took a quick look and find it is due to the incorrect result from isLayoutIdentical too

[ChuanqiXu9]

then the type of the initializer is not considered to be an union, so the computed size is not correct, which is the reason for the crash.

Have you considered making isLayoutIdentical more robust? It's possible we might need to use layoutInfo for taking some of the decisions (it's lazily computed and can be augmented to do more work), etc. It's probably best if we can hide this extra logic and keep some more similarity to OG skeleton to the extend we can.

It may not solve the root problem. I spent some time to think about this and my conclusion is:

1. We may not have very similar skeleton for the particular problem. Since the type for union in LLVM and CIR are fundamentally different.
2. To solve the problem, we might have to have two type systems. One for CIRGen and one for LLVM lowering.

Here is the story:

1. In CIRGen, we hope the different expression to initialize the union to have the same type (isLayoutIdentical). This is what addressed by the previous PR.
2. But the problem comes when the initialized value is not the largest member, then we have to have some paddings.
3. Then in LLVM lowering, we will convert the union type to the largest type (i64 in this example) and we have members like i32 and paddings (array i8 4). And now we have problems for how to initialize i64 with i32 and a array of i8 4.

Previously, it just works because the type of the initializer is not considered to be an union type. It makes sense in LLVM since union type is not a thing in LLVM. But it causes problem in the previous PR.