-
Notifications
You must be signed in to change notification settings - Fork 188
[CIR] Implement static local variable initialization with guard variables #2046
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
base: gh/lanza/20/base
Are you sure you want to change the base?
Conversation
…bles This implements thread-safe initialization of function-local static variables with dynamic initializers, following the Itanium C++ ABI. The implementation: 1. Adds `static_local` attribute to GlobalOp and GetGlobalOp to mark function-local statics requiring guarded initialization 2. Implements guarded initialization in LoweringPrepare pass using __cxa_guard_acquire/__cxa_guard_release for thread safety 3. Adds helper methods createIsNull/createIsNotNull to CIRBaseBuilder 4. Implements emitGuardedInit in CIRGenItaniumCXXABI to emit the ctor region and mark the global as static_local 5. Adds mangleStaticGuardVariable to ASTAttrInterfaces for mangling The guard variable pattern follows Itanium ABI 3.3.2: - First byte of guard checked with acquire load for fast path - __cxa_guard_acquire called if uninitialized - __cxa_guard_release called after successful initialization ghstack-source-id: 6081423 Pull-Request: #2046
…bles This implements thread-safe initialization of function-local static variables with dynamic initializers, following the Itanium C++ ABI. The implementation: 1. Adds `static_local` attribute to GlobalOp and GetGlobalOp to mark function-local statics requiring guarded initialization 2. Implements guarded initialization in LoweringPrepare pass using __cxa_guard_acquire/__cxa_guard_release for thread safety 3. Adds helper methods createIsNull/createIsNotNull to CIRBaseBuilder 4. Implements emitGuardedInit in CIRGenItaniumCXXABI to emit the ctor region and mark the global as static_local 5. Adds mangleStaticGuardVariable to ASTAttrInterfaces for mangling The guard variable pattern follows Itanium ABI 3.3.2: - First byte of guard checked with acquire load for fast path - __cxa_guard_acquire called if uninitialized - __cxa_guard_release called after successful initialization ghstack-source-id: bf90e4f Pull-Request: #2046
…bles This implements thread-safe initialization of function-local static variables with dynamic initializers, following the Itanium C++ ABI. The implementation: 1. Adds `static_local` attribute to GlobalOp and GetGlobalOp to mark function-local statics requiring guarded initialization 2. Implements guarded initialization in LoweringPrepare pass using __cxa_guard_acquire/__cxa_guard_release for thread safety 3. Adds helper methods createIsNull/createIsNotNull to CIRBaseBuilder 4. Implements emitGuardedInit in CIRGenItaniumCXXABI to emit the ctor region and mark the global as static_local 5. Adds mangleStaticGuardVariable to ASTAttrInterfaces for mangling The guard variable pattern follows Itanium ABI 3.3.2: - First byte of guard checked with acquire load for fast path - __cxa_guard_acquire called if uninitialized - __cxa_guard_release called after successful initialization ghstack-source-id: 272ef4e Pull-Request: #2046
…bles This implements thread-safe initialization of function-local static variables with dynamic initializers, following the Itanium C++ ABI. The implementation: 1. Adds `static_local` attribute to GlobalOp and GetGlobalOp to mark function-local statics requiring guarded initialization 2. Implements guarded initialization in LoweringPrepare pass using __cxa_guard_acquire/__cxa_guard_release for thread safety 3. Adds helper methods createIsNull/createIsNotNull to CIRBaseBuilder 4. Implements emitGuardedInit in CIRGenItaniumCXXABI to emit the ctor region and mark the global as static_local 5. Adds mangleStaticGuardVariable to ASTAttrInterfaces for mangling The guard variable pattern follows Itanium ABI 3.3.2: - First byte of guard checked with acquire load for fast path - __cxa_guard_acquire called if uninitialized - __cxa_guard_release called after successful initialization ghstack-source-id: 29329f1 Pull-Request: #2046
bcardosolopes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this looks mostly good. Please add a .cir tests for parsing/printing. More comments inline
| oldGlobalOp.getName(), typedInit.getType(), | ||
| oldGlobalOp.getConstant(), globalOp.getLinkage()); | ||
| // FIXME(cir): OG codegen inserts new GV before old one, we probably don't | ||
| // need that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM translation usually put them out in the proper order, because of the way MLIR handles this I don't think it should be a problem for us.
| void emitCXXGuardedInit(const VarDecl &varDecl, cir::GlobalOp globalOp, | ||
| bool performInit); | ||
|
|
||
| /// EmitCXXGlobalVarDeclInit - Create the initializer for a C++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the EmitCXXGlobalVarDeclInit - in the comment
| // CIR: cir.global "private" internal dso_local static_local @_ZZ3foovE3val = #cir.int<0> : !s32i {alignment = 4 : i64, ast = #cir.var.decl.ast} | ||
| // CIR: cir.global "private" internal dso_local @_ZGVZ3foovE3val = #cir.int<0> : !s64i {alignment = 8 : i64} | ||
| // CIR: cir.func {{.*}} @_Z3foov() extra(#fn_attr) { | ||
| // CIR-NEXT: %0 = cir.get_global static_local @_ZZ3foovE3val : !cir.ptr<!s32i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of our tests should have literal SSA values, regex it all please!
| // CIRGEN-NEXT: %1 = cir.call @_Z3fnAv() : () -> !s32i | ||
| // CIRGEN-NEXT: cir.store align(4) %1, %0 : !s32i, !cir.ptr<!s32i> | ||
| // CIRGEN-NEXT: } {alignment = 4 : i64, ast = #cir.var.decl.ast} | ||
| // CIRGEN-NEXT: cir.func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for cir.func should only be CIRGEN since it's a coincidence it shows up next but it isn't required to do so. Also add the name of the function too.
|
|
||
| // CIRGEN: cir.func private @_Z3fnAv() -> !s32i | ||
| // CIRGEN: cir.global "private" internal dso_local static_local @_ZZ3foovE3val = ctor : !s32i { | ||
| // CIRGEN-NEXT: %0 = cir.get_global @_ZZ3foovE3val : !cir.ptr<!s32i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having two instances of cir.get_global @_ZZ3foovE3val inside the initializer and another one outside with the specific keyword is a bit confusing, however still pretty handy when unwrapping so that lowering prepare could be more direct.
Since there's still more work in this area, better not over design too early, but we should revisit this in the future. Can you add a comment in the CIRGen code that generates this get_global so we can later take other things into consideration?
Stack from ghstack (oldest at bottom):
This implements thread-safe initialization of function-local static
variables with dynamic initializers, following the Itanium C++ ABI.
The implementation:
static_localattribute to GlobalOp and GetGlobalOp to markfunction-local statics requiring guarded initialization
__cxa_guard_acquire/__cxa_guard_release for thread safety
ctor region and mark the global as static_local
The guard variable pattern follows Itanium ABI 3.3.2: