Skip to content

Conversation

@lanza
Copy link
Member

@lanza lanza commented Oct 30, 2024

We diverge from CodeGen here by delaying the function emission that
happens for a global variable. However, due to situations where a global
can be emitted while building out a function the old CGF might not be
invalid. So we need to store it here just in case.

lanza added 2 commits October 29, 2024 20:14
Created using spr 1.3.5
@lanza lanza requested a review from bcardosolopes as a code owner October 30, 2024 00:14
@smeenai
Copy link
Collaborator

smeenai commented Oct 30, 2024

Is there any way to write a test case for this, and should it use llvm::SaveAndRestore instead to work for all exit paths?

@lanza
Copy link
Member Author

lanza commented Oct 30, 2024

Is there any way to write a test case for this

Not yet, I'm trying to hoist out a bunch of code from a 1000 line patch for static local variables that needs heavily refactored... hence all this string of PRs. But this occurs during non-trivial static locals here for something like this:

int foo();
int bar() {
  static int a = foo();
  return a;
}

which we'll emit (before LoweringPrepare) as something like

cir.global "private"  internal dsolocal @_ZZ3barvE1a !s32i {alignment = 4 : i64, static_local = true }
cir.func  @_Z3barv() -> !s32i {
    %1 = cir.static_local "_ZZ3barvE1a" !cir.ptr<1s32i>
    %2 = cir.load %1 : !cir.ptr<!s32i>, !s32i
    cir.return %3 : !s32i
} 

should it use llvm::SaveAndRestore instead to work for all exit paths?

Yes, couldn't remember the name of this mechanism and just did it manually. Thanks!

lanza added 3 commits October 29, 2024 20:59
Created using spr 1.3.5

[skip ci]
Created using spr 1.3.5
Created using spr 1.3.5
@smeenai
Copy link
Collaborator

smeenai commented Oct 30, 2024

Is there any way to write a test case for this

Not yet, I'm trying to hoist out a bunch of code from a 1000 line patch for static local variables that needs heavily refactored... hence all this string of PRs. But this occurs during non-trivial static locals here for something like this:

int foo();
int bar() {
  static int a = foo();
  return a;
}

which we'll emit (before LoweringPrepare) as something like

cir.global "private"  internal dsolocal @_ZZ3barvE1a !s32i {alignment = 4 : i64, static_local = true }
cir.func  @_Z3barv() -> !s32i {
    %1 = cir.static_local "_ZZ3barvE1a" !cir.ptr<1s32i>
    %2 = cir.load %1 : !cir.ptr<!s32i>, !s32i
    cir.return %3 : !s32i
} 

Got it, LGTM then. Not related to this patch, but shouldn't the cir.global have the initializer associated with it?

lanza added 2 commits October 30, 2024 13:08
Created using spr 1.3.5

[skip ci]
Created using spr 1.3.5
@lanza lanza changed the base branch from spr/lanza/main.circodegen-store-the-old-cirgenfunction-when-popping-to-a-new-one to main October 30, 2024 17:08
@lanza lanza merged commit 91b172f into main Oct 30, 2024
7 checks passed
@lanza lanza deleted the spr/lanza/circodegen-store-the-old-cirgenfunction-when-popping-to-a-new-one branch October 30, 2024 17:08
lanza added a commit that referenced this pull request Nov 5, 2024
We diverge from CodeGen here by delaying the function emission that
happens for a global variable. However, due to situations where a global
can be emitted while building out a function the old CGF might not be
invalid. So we need to store it here just in case.

Reviewers: bcardosolopes, smeenai

Reviewed By: smeenai

Pull Request: #1023
lanza added a commit that referenced this pull request Mar 18, 2025
We diverge from CodeGen here by delaying the function emission that
happens for a global variable. However, due to situations where a global
can be emitted while building out a function the old CGF might not be
invalid. So we need to store it here just in case.

Reviewers: bcardosolopes, smeenai

Reviewed By: smeenai

Pull Request: #1023
lanza added a commit to lanza/llvm-project that referenced this pull request Aug 11, 2025
We diverge from CodeGen here by delaying the function emission that
happens for a global variable. However, due to situations where a global
can be emitted while building out a function the old CGF might not be
invalid. So we need to store it here just in case.

Reviewers: bcardosolopes, smeenai

Reviewed By: smeenai

Pull Request: llvm/clangir#1023
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