efriedma added inline comments.
Herald added subscribers: bd1976llvm, jplehr.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:520
+ getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit))
+ return;
+
----------------
efriedma wrote:
> I don't really like the copy-pasted bits here.
It would be nice it we could to unify EmitCXXCtorInit with
EmitCXXGlobalVarDeclInitFunc a bit more.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006
+ if (NeedsGlobalCtor || NeedsGlobalDtor)
+ DelayedCXXInitPosition[D] = ~0U;
+ } else {
----------------
zahiraam wrote:
> efriedma wrote:
> > zahiraam wrote:
> > > efriedma wrote:
> > > > zahiraam wrote:
> > > > > efriedma wrote:
> > > > > > zahiraam wrote:
> > > > > > > efriedma wrote:
> > > > > > > > zahiraam wrote:
> > > > > > > > > Do you agree this should be done only when one of those flags
> > > > > > > > > is on?
> > > > > > > > Yes, that's fine; I wasn't really paying close attention to the
> > > > > > > > exact code. Just wanted to make the point about the structure
> > > > > > > > of the if statements, and code was the easiest way to explain
> > > > > > > > it.
> > > > > > > >
> > > > > > > > Maybe the outer if statement should actually be `if
> > > > > > > > (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor) {`.
> > > > > > > >
> > > > > > > > On a related note, we might want to avoid the name "ctor", in
> > > > > > > > case that accidentally conflicts with some user code; an
> > > > > > > > "__"-prefixed name would be appropriate.
> > > > > > > >> Maybe the outer if statement should actually be if
> > > > > > > >> (isStaticInit(D, getLangOpts()) && NeedsGlobalCtor) {
> > > > > > > Not sure about that! There are cases where (isStaticInit(D,
> > > > > > > getLangOpts())) = true and NeedsGlobalCtor=false, but
> > > > > > > NeedsGlobalDtor=true. In which case a __dtor needs to be emitted,
> > > > > > > no?
> > > > > > >
> > > > > > > Writing the condition as you are proposing would actually not get
> > > > > > > me into the body to emit the __dtor. Is that what we want?
> > > > > > EmitCXXGlobalVarDeclInitFunc should be able to handle that case.
> > > > > >
> > > > > > Looking again, I'm a little concerned that in the isStaticInit()
> > > > > > case, we're skipping a bunch of the logic in
> > > > > > EmitCXXGlobalVarDeclInitFunc. EmitCXXCtorInit handles the basic
> > > > > > cases correctly, but there are a lot of special cases in
> > > > > > EmitCXXGlobalVarDeclInitFunc.
> > > > > I have left the condition as it was to make sure no cases are left.
> > > > > What other cases are you thinking of?
> > > > >
> > > > EmitCXXGlobalVarDeclInitFunc has special cases for CUDA, OpenMP,
> > > > thread-local variables, the InitSeg attribute, and inline variables.
> > > Let me know if this works? I have added one additional LIT test. I
> > > referred to
> > > https://learn.microsoft.com/en-us/cpp/cpp/storage-classes-cpp?view=msvc-170#thread_local
> > > for thread_local.
> > > For the inline variables, I couldn't find much information about
> > > initialization of inline variables. I tried to find a case that wouldn't
> > > pass by the new code and wouldn't pass by EmitCXXGlobalVarDeclInitFunc,
> > > but no success.
> > Inline variable example:
> >
> > ```
> > __declspec(dllimport) extern int x;
> > inline int *y = &x;
> > int **z = &y;
> > ```
> >
> > This should trigger your new codepath, I think?
> Yes. And the IR looks like this:
>
> @"?y@@3PEAHEA" = linkonce_odr dso_local global ptr null, comdat, align 8
> @"?z@@3PEAPEAHEA" = dso_local global ptr @"?y@@3PEAHEA", align 8
> @"?x@@3HA" = external dllimport global i32, align 4
> @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr,
> ptr } { i32 201, ptr @__ctor, ptr null }]
>
> ; Function Attrs: noinline nounwind
> define linkonce_odr dso_local void @__ctor() #0 comdat {
> entry:
> store ptr @"?x@@3HA", ptr @"?y@@3PEAHEA", align 8
> ret void
> }
>
> I think this is correct.
> Adding it to the LIT testing.
Without your patch, the IR looks significantly different; the constructor
function is named ??__Ey@@YAXXZ, and there's a reference to the variable in
llvm.global_ctors. This is necessary to ensure we only construct the variable
once.
In this particular case, I guess the harm of constructing the variable multiple
times would be relatively minor, but it's still not a great idea. And we
definitely can't emit a linkonce_odr function and just name it "__ctor".
================
Comment at: clang/test/CodeGenCXX/constant-init.cpp:17
+
+// CHECK: define internal void @"??__Ft2@@YAXXZ"() #0 {
+// CHECK: entry:
----------------
zahiraam wrote:
> efriedma wrote:
> > This doesn't look like your new codepath is triggering? If your code is
> > working correctly, it should, I think?
> That's right. In all cases where
>
> llvm::Constant *Initializer = emitter->tryEmitForInitializer(*InitDecl);
>
> is returning an initializer (with NeedsGlobalCtor=false and
> NeedsGlobalDtor=true) the control path is following the old path (no call to
> EmitCXXCtorInit), i.e. call to EmitCXXGlobalVarDeclInitFunc.
>
> Unless we want to change the value of NeedsGlobaCtor in this case?
>
> With:
> diff --git a/clang/lib/CodeGen/CodeGenModule.cpp
> b/clang/lib/CodeGen/CodeGenModule.cpp
> index f7add1350238..106ac355d356 100644
> --- a/clang/lib/CodeGen/CodeGenModule.cpp
> +++ b/clang/lib/CodeGen/CodeGenModule.cpp
> @@ -4898,6 +4898,8 @@ void CodeGenModule::EmitGlobalVarDefinition(const
> VarDecl *D,
> // also don't need to register a destructor.
> if (getLangOpts().CPlusPlus && !NeedsGlobalDtor)
> DelayedCXXInitPosition.erase(D);
> + if (isStaticInit(D, getLangOpts()) && !NeedsGlobalCtor)
> + NeedsGlobalCtor = true;
>
> #ifndef NDEBUG
> CharUnits VarSize = getContext().getTypeSizeInChars(ASTTy) +
>
> In which case for:
>
> struct Test1 {
> constexpr Test1(int) {}
> ~Test1() {}
> };
> inline constinit Test1 t2 = 2;
>
> We would get this IR?
>
> @llvm.global_ctors = appending global [2 x { i32, ptr, ptr }] [{ i32, ptr,
> ptr } { i32 201, ptr @__ctor, ptr null }, { i32, ptr, ptr } { i32 65535, ptr
> @__dtor, ptr null }]
>
> ; Function Attrs: noinline nounwind
> define linkonce_odr dso_local void @__ctor() #0 comdat {
> entry:
> %call = call noundef ptr @"??0Test1@@QEAA@H@Z"(ptr noundef nonnull align 1
> dereferenceable(1) @"?t2@@3UTest1@@A", i32 noundef 2)
> %0 = call i32 @atexit(ptr @"??__Ft2@@YAXXZ") #2
> ret void
> }
>
> define internal void @"??__Ft2@@YAXXZ"() #0 {
> entry:
> call void @"??1Test1@@QEAA@XZ"(ptr @"?t2@@3UTest1@@A")
> ret void
> }
>
> define linkonce_odr dso_local void @__dtor() #0 comdat {
> entry:
> %0 = call i32 @atexit(ptr @"??__Ft2@@YAXXZ.1") #2
> ret void
> }
>
Reading this again, not sure what I was asking for here.
We should not be calling atexit() from a priority 201 constructor; we should
always register it with the normal registration priority, so stuff is destroyed
in the right order.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137107/new/
https://reviews.llvm.org/D137107
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
