[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam marked an inline comment as done.
zahiraam added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5006
+    if (NeedsGlobalCtor || NeedsGlobalDtor)
+      DelayedCXXInitPosition[D] = ~0U;
+  } else {
----------------
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.


================
Comment at: clang/test/CodeGenCXX/constant-init.cpp:17
+
+// CHECK: define internal void @"??__Ft2@@YAXXZ"() #0 {
+// CHECK: entry:
----------------
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 at 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 at 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
}



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137107/new/

https://reviews.llvm.org/D137107