Const array of structs may have different element types

[gitoleg]

I faced with the problem: the next code fails in the lowering due to type mismatch of the llvm.insertvalue operation.

typedef struct {
     long a0;
     int a1;
} A;

typedef struct {
     int b0;
     A b1[1];
} B;

B b[2] = {
    {1, {0, 1} },

    {1, {0, 0} }
};

The reason is that elements of this array have different types, though it sounds a bit weird - but it's true! - and correspond to OG. This is how the OG LLVM IR looks like: @b = dso_local global <{ ... . The point is b has struct type. But in CIR we still have an array type for b- that's why we get a fail.

Well, actually it's not hard to fix this problem (though it was hard to find where to fix). Basically the condition here guard CommonElementType from being assigned to nullptr. But looks like the straightforward fix will break nested-union-array test - and instead of array of structs we'll get a new anon struct type and LLVM IR difference as well.

So the questions is - the changes introduced in #1236 were caused by some new detected fails? Or just by the desire to get the same LLVM IR for this case?

@bcardosolopes @ChuanqiXu9

[ChuanqiXu9]

I think the intuition is to try to make the element type of array to be the same type in CIR. It is odd to have different types in an array. Maybe we shouldn't use ArrayAttr for such initialization style in the first place.

[gitoleg]

Yes, it's odd. Well my point was that not every C array is LLVM IR array - as in the example where no CIR is involved. Sometimes C arrays may be represented as anon struct types.

I'm trying to collect opinions - what's a good way here? Maybe we agree with some LLVM IR difference. Or seek for the better workaround?

So is the case in the nested-union-array.c is the only you faced with? If it's so we can try to postpone the solution for some time and handle this case somehow better - e.g. seek for nested union types.

PS Still think that separate union type is what do we need - too many problems caused by unions :(

[tommymcm]

Reviving this issue because I've hit the same failed assertion in a SPEC benchmark. The changes introduced in #1236 doesn't actually match OG CodeGen:

CIR-to-LLVM:

@data = constant [2 x { { ptr } }] 

OG CodeGen:

@data = constant [2 x %struct.nested]

If I extend the nested-union-array.c test to include an additional non-pointer member, then OG CodeGen doesn't match the intent behind the relevant PR.

Extended Test:

struct nested
{
  union {
    const char *single;
    const char *const *multi;
    long int integer;
  } output;
};
const struct nested data[] = 
{ { { .multi = test, }, },
  { { .single = "hello", }, },
  { { .integer = 10 } }
};

OG CodeGen:

@data = dso_local constant <{ %struct.nested, %struct.nested, { { i64 } } }>

I'm planning to open a PR to revert the changes and remove the test.

@ChuanqiXu9 did the relevant PR fix a bug somewhere for you? If so, could you send me a more detailed test to make sure you don't experience any regressions?

[ChuanqiXu9]

Reviving this issue because I've hit the same failed assertion in a SPEC benchmark. The changes introduced in #1236 doesn't actually match OG CodeGen:

CIR-to-LLVM:

@data = constant [2 x { { ptr } }] 

OG CodeGen:

@data = constant [2 x %struct.nested]

If I extend the nested-union-array.c test to include an additional non-pointer member, then OG CodeGen doesn't match the intent behind the relevant PR.

Extended Test:

struct nested
{
  union {
    const char *single;
    const char *const *multi;
    long int integer;
  } output;
};
const struct nested data[] = 
{ { { .multi = test, }, },
  { { .single = "hello", }, },
  { { .integer = 10 } }
};

OG CodeGen:

@data = dso_local constant <{ %struct.nested, %struct.nested, { { i64 } } }>

I'm planning to open a PR to revert the changes and remove the test.

@ChuanqiXu9 did the relevant PR fix a bug somewhere for you? If so, could you send me a more detailed test to make sure you don't experience any regressions?

Thanks. The only test I am using is the C set of Spec2017. I didn't work on this now. If you want, you can test it with the C set of Spec2017.

[bcardosolopes]

Thanks for looking into this @tommymcm