Skip to content

[CIR][CodeGen] Updates GlobalViewAttr's indices computation for the union type #1584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

gitoleg
Copy link
Collaborator

@gitoleg gitoleg commented Apr 23, 2025

This PR introduces a new attribute for the explicit byte offset for GlobalViewAttr. It's a proposal, so feel free to reject it once it's not good from your point of view.

The problem is (as usually) with globals and unions: looks like we can not really use indexes in the GlobalView to address an array of unions. For example, the next program prints 4 now, but it should be 42:

typedef struct {
    long s0;
    int  s1;
} S;

typedef union {
   int  f0;
   S f1;
} U;

static U g1[3] = {{42},{42},{42}};
int* g2 = &g1[1].f1.s1;

int main() {       
   (*g2) = 4;
   printf("%d\n", g1[1].f0);    
   return0;
}

The problem is that we compute wrong indices in CIRGenBuilder::computeGlobalViewIndicesFromFlatOffset. Maybe it can be even fixed in this case, but I have a feeling that the fix would be a bit fragile.

So instead of trying to support indexes for the array of unions I suggest to use the offset explicitly.

From the implementation point of view there are some changes in CIRGenBuilder - but nothing really new is in there - I just did not want to introduce copy-pasta for the isOffsetInUnion function that is pretty the same as former computeGlobalViewIndicesFromFlatOffset.

@gitoleg
Copy link
Collaborator Author

gitoleg commented Apr 24, 2025

I think I have a better solution and just update the index computation - so once you are good with it - I can rename the PR.
The only downside of this approach and it could be a chance that indices will be computed wrong:

typedef struct {
   double x;
   int y;
} S1;

typedef struct {
     long x;
     int y;
} S2; 

typedef union {
   S1 s1;
   S2 s2;
} U;

static U g1[2] = {{42},{42}};
int* g2 = &g1[1].s1.x;
int* g3 = &g1[1].s2.x;

Both of g2 and g3 will have the same indexes. But the point is - both of them will have the same offset on LLVM IR side, which is more important since it's correct and even with the original LLVM with CIR involved.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, apologies for the delay! LGTM pending some nits

@@ -95,15 +95,17 @@ void CIRGenBuilderTy::computeGlobalViewIndicesFromFlatOffset(
unsigned AlignMask = Layout.getABITypeAlign(Elts[I]).value() - 1;
if (RecordTy.getPacked())
AlignMask = 0;
Pos = (Pos + AlignMask) & ~AlignMask;
if (!RecordTy.isUnion())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some comment here so it's easy to understand why unions are different?

assert(Offset >= 0);
if (Offset < Pos + EltSize) {
Indices.push_back(I);
SubType = Elts[I];
Offset -= Pos;
break;
}
Pos += EltSize;
if (!RecordTy.isUnion())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here!

@gitoleg gitoleg changed the title [CIR][CodeGen] Introduce bytes offset attribute [CIR][CodeGen] Updates GlobalViewAttr's indices computation for the union type May 5, 2025
@gitoleg
Copy link
Collaborator Author

gitoleg commented May 5, 2025

done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants