efriedma added inline comments.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:520
+ getOpenMPRuntime().emitDeclareTargetVarDefinition(D, Addr, PerformInit))
+ return;
+
----------------
I don't really like the copy-pasted bits here.
================
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:
> > > > > > > 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?
================
Comment at: clang/test/CodeGenCXX/constant-init.cpp:17
+
+// CHECK: define internal void @"??__Ft2@@YAXXZ"() #0 {
+// CHECK: entry:
----------------
This doesn't look like your new codepath is triggering? If your code is
working correctly, it should, I think?
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
