[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Yuanfang Chen via Phabricator via cfe-commits]

ychen abandoned this revision.
ychen added a comment.

Superseded by  D127233 , D127259 
, rG7f8d844df5e9 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D126341#3554286 , @rnk wrote:

> Well, I guess we're out of luck, but that seems like a very poorly considered 
> requirement from the standard. If we can't use comdats for inline variables, 
> every time you include a header with a dynamically initialized variable, it 
> will generate extra initialization code in every TU that cannot be optimized 
> away. This reminds me of the problems people used to have where every TU 
> including  emitted extra initialization code.
>
> I think we have two options:
>
> 1. Full conformance: Stop using comdats altogether and suffer costs in code 
> size and startup time
> 2. Partial / compromised conformance: Provide ordering guarantees between 
> global_ctors entries so that we can ensure that inline variables in headers 
> have the expected initialization order

I'd prefer option 2 due to the code size/startup time cost. About enforcing the 
order in global_ctors (and maybe also global_dtors), how about adding an 
integer `order` field to each entry in global_ctors. Then let the IR verifier 
check the order is non-descending (passes are allowed to reorder entries with 
the same `order`). Another guarantee needed for option 2 is that the linker 
consistently picks the first COMDAT. I think this is getting out of our hands 
but this assumption *could* be made in practice? In very rare cases, if a 
linker changes this behavior, we'll have to rely on user reports to find out.




Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563
+  } else if (IsInstantiation ||
  getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
  D->hasAttr()) {

rsmith wrote:
> rnk wrote:
> > rsmith wrote:
> > > rnk wrote:
> > > > @rsmith , if inline global variable initialization has ordering 
> > > > requirements, we have a bug, because I believe this GVA_DiscardableODR 
> > > > codepath handles them, and we come through here to give them separate 
> > > > initializers in llvm.global_ctors. See the example with two separate 
> > > > global_ctors entries on godbolt:
> > > > https://gcc.godbolt.org/z/5d577snqb
> > > > 
> > > > As long as LLVM doesn't provide ordering guarantees about same priority 
> > > > initializers in global_ctors, inline globals have the same problems as 
> > > > template instantiations. IMO whatever solution we use to order inline 
> > > > globals should be used for template instantiations.
> > > > 
> > > > Intuitively, that means LLVM should promise to run global_ctors in 
> > > > left-to-right order, and if all TUs instantiate initializers in the 
> > > > same order, everything should behave intuitively.
> > > > 
> > > > The question then becomes, why doesn't this work already?
> > > > Intuitively, that means LLVM should promise to run global_ctors in 
> > > > left-to-right order
> > > 
> > > I don't think that's sufficient, due to the way we use COMDATs to discard 
> > > duplicate global initializers. Consider:
> > > 
> > > TU 1 defines inline variable A.
> > > TU 2 defines inline variable A and then inline variable B.
> > > The standard guarantees that A is initialized before B in this scenario. 
> > > But if (somehow) the linker picks the definition of A from TU 2, but 
> > > orders the initializers from TU 1 first, then the resulting global_ctors 
> > > order will be B then A, which is not allowed.
> > > 
> > > Either we need a guarantee that linkers will use the same ordering 
> > > between objects when picking COMDATs as when concatenating `.init_array`, 
> > > or we need to stop using the COMDAT trick for them (and either make LLVM 
> > > respect the `@llvm.global_ctors` order or coalesce all inline variable 
> > > initializers into the same function we run non-inline initializers from 
> > > or something). Getting that guarantee seems like the best path to me, 
> > > since the other option will presumably mean we check the guard variable 
> > > once on startup for each TU that defines the variable, and it's something 
> > > I expect linkers already happen to guarantee in practice.
> > > 
> > > > IMO whatever solution we use to order inline globals should be used for 
> > > > template instantiations.
> > > 
> > > That sounds like it would regress what globalopt is able to optimize, for 
> > > no gain in conformance nor perhaps any real gain in initialization order 
> > > guarantees. The inline variable case is different, both because we can 
> > > guarantee an initialization order and because the standard requires us to 
> > > do so. Should we add complexity to the compiler whose only purpose is to 
> > > mask bugs, if that complexity doesn't actually define away the 
> > > possibility of bad behavior?
> > > 
> > > If we can actually describe a rule that we provide for initialization 
> > > order of instantiated variables, and we can easily implement that rule 
> > > and be confident we won't want to substantially weaken it later, and 

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Well, I guess we're out of luck, but that seems like a very poorly considered 
requirement from the standard. If we can't use comdats for inline variables, 
every time you include a header with a dynamically initialized variable, it 
will generate extra initialization code in every TU that cannot be optimized 
away. This reminds me of the problems people used to have where every TU 
including  emitted extra initialization code.

I think we have two options:

1. Full conformance: Stop using comdats altogether and suffer costs in code 
size and startup time
2. Partial / compromised conformance: Provide ordering guarantees between 
global_ctors entries so that we can ensure that inline variables in headers 
have the expected initialization order


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563
+  } else if (IsInstantiation ||
  getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
  D->hasAttr()) {

rnk wrote:
> rsmith wrote:
> > rnk wrote:
> > > @rsmith , if inline global variable initialization has ordering 
> > > requirements, we have a bug, because I believe this GVA_DiscardableODR 
> > > codepath handles them, and we come through here to give them separate 
> > > initializers in llvm.global_ctors. See the example with two separate 
> > > global_ctors entries on godbolt:
> > > https://gcc.godbolt.org/z/5d577snqb
> > > 
> > > As long as LLVM doesn't provide ordering guarantees about same priority 
> > > initializers in global_ctors, inline globals have the same problems as 
> > > template instantiations. IMO whatever solution we use to order inline 
> > > globals should be used for template instantiations.
> > > 
> > > Intuitively, that means LLVM should promise to run global_ctors in 
> > > left-to-right order, and if all TUs instantiate initializers in the same 
> > > order, everything should behave intuitively.
> > > 
> > > The question then becomes, why doesn't this work already?
> > > Intuitively, that means LLVM should promise to run global_ctors in 
> > > left-to-right order
> > 
> > I don't think that's sufficient, due to the way we use COMDATs to discard 
> > duplicate global initializers. Consider:
> > 
> > TU 1 defines inline variable A.
> > TU 2 defines inline variable A and then inline variable B.
> > The standard guarantees that A is initialized before B in this scenario. 
> > But if (somehow) the linker picks the definition of A from TU 2, but orders 
> > the initializers from TU 1 first, then the resulting global_ctors order 
> > will be B then A, which is not allowed.
> > 
> > Either we need a guarantee that linkers will use the same ordering between 
> > objects when picking COMDATs as when concatenating `.init_array`, or we 
> > need to stop using the COMDAT trick for them (and either make LLVM respect 
> > the `@llvm.global_ctors` order or coalesce all inline variable initializers 
> > into the same function we run non-inline initializers from or something). 
> > Getting that guarantee seems like the best path to me, since the other 
> > option will presumably mean we check the guard variable once on startup for 
> > each TU that defines the variable, and it's something I expect linkers 
> > already happen to guarantee in practice.
> > 
> > > IMO whatever solution we use to order inline globals should be used for 
> > > template instantiations.
> > 
> > That sounds like it would regress what globalopt is able to optimize, for 
> > no gain in conformance nor perhaps any real gain in initialization order 
> > guarantees. The inline variable case is different, both because we can 
> > guarantee an initialization order and because the standard requires us to 
> > do so. Should we add complexity to the compiler whose only purpose is to 
> > mask bugs, if that complexity doesn't actually define away the possibility 
> > of bad behavior?
> > 
> > If we can actually describe a rule that we provide for initialization order 
> > of instantiated variables, and we can easily implement that rule and be 
> > confident we won't want to substantially weaken it later, and we can 
> > thereby assure our users that we will satisfy that rule, then I think that 
> > could be interesting, but anything less than that doesn't seem worthwhile 
> > to me.
> > 
> > > The question then becomes, why doesn't this work already?
> > 
> > It looks like it mostly does.
> > 
> > One factor here is instantiation order. When we instantiate a variable, we 
> > add the variables that it references to our "to be instantiated" list, 
> > which is processed later:
> > ```
> > template int Fib = Fib + Fib;
> > template<> int Fib<0> = 0;
> > template<> int Fib<1> = 1;
> > ```
> > * instantiating `Fib<5>` will append `Fib<3>` and `Fib<4>` to the list
> > * then we visit `Fib<3>` and append `Fib<1>` and `Fib<2>` to the list
> > * then we visit `Fib<4>` and add no new entries
> > 
> > We pass declarations to the consumer when we're done with the instantiation 
> > step. *Sometimes* this includes instantiating variables referenced by that 
> > variable, and *sometimes* it doesn't. The difference is whether we're 
> > performing "recursive" instantiation or not.
> > 
> > When we're performing immediate instantiation of a variable (either because 
> > it was explicitly instantiated, or because we might need its value 
> > immediately because it might be usable in constant expressions), our 
> > instantiation step is non-recursive. We just add declarations to `Sema`'s 
> > "to be instantiated at end of TU" list. This is at least a little important 
> > semantically: we allow a matching specialization to be declared after the 
> > first use wherever possible. In that case, 

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563
+  } else if (IsInstantiation ||
  getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
  D->hasAttr()) {

rsmith wrote:
> rnk wrote:
> > @rsmith , if inline global variable initialization has ordering 
> > requirements, we have a bug, because I believe this GVA_DiscardableODR 
> > codepath handles them, and we come through here to give them separate 
> > initializers in llvm.global_ctors. See the example with two separate 
> > global_ctors entries on godbolt:
> > https://gcc.godbolt.org/z/5d577snqb
> > 
> > As long as LLVM doesn't provide ordering guarantees about same priority 
> > initializers in global_ctors, inline globals have the same problems as 
> > template instantiations. IMO whatever solution we use to order inline 
> > globals should be used for template instantiations.
> > 
> > Intuitively, that means LLVM should promise to run global_ctors in 
> > left-to-right order, and if all TUs instantiate initializers in the same 
> > order, everything should behave intuitively.
> > 
> > The question then becomes, why doesn't this work already?
> > Intuitively, that means LLVM should promise to run global_ctors in 
> > left-to-right order
> 
> I don't think that's sufficient, due to the way we use COMDATs to discard 
> duplicate global initializers. Consider:
> 
> TU 1 defines inline variable A.
> TU 2 defines inline variable A and then inline variable B.
> The standard guarantees that A is initialized before B in this scenario. But 
> if (somehow) the linker picks the definition of A from TU 2, but orders the 
> initializers from TU 1 first, then the resulting global_ctors order will be B 
> then A, which is not allowed.
> 
> Either we need a guarantee that linkers will use the same ordering between 
> objects when picking COMDATs as when concatenating `.init_array`, or we need 
> to stop using the COMDAT trick for them (and either make LLVM respect the 
> `@llvm.global_ctors` order or coalesce all inline variable initializers into 
> the same function we run non-inline initializers from or something). Getting 
> that guarantee seems like the best path to me, since the other option will 
> presumably mean we check the guard variable once on startup for each TU that 
> defines the variable, and it's something I expect linkers already happen to 
> guarantee in practice.
> 
> > IMO whatever solution we use to order inline globals should be used for 
> > template instantiations.
> 
> That sounds like it would regress what globalopt is able to optimize, for no 
> gain in conformance nor perhaps any real gain in initialization order 
> guarantees. The inline variable case is different, both because we can 
> guarantee an initialization order and because the standard requires us to do 
> so. Should we add complexity to the compiler whose only purpose is to mask 
> bugs, if that complexity doesn't actually define away the possibility of bad 
> behavior?
> 
> If we can actually describe a rule that we provide for initialization order 
> of instantiated variables, and we can easily implement that rule and be 
> confident we won't want to substantially weaken it later, and we can thereby 
> assure our users that we will satisfy that rule, then I think that could be 
> interesting, but anything less than that doesn't seem worthwhile to me.
> 
> > The question then becomes, why doesn't this work already?
> 
> It looks like it mostly does.
> 
> One factor here is instantiation order. When we instantiate a variable, we 
> add the variables that it references to our "to be instantiated" list, which 
> is processed later:
> ```
> template int Fib = Fib + Fib;
> template<> int Fib<0> = 0;
> template<> int Fib<1> = 1;
> ```
> * instantiating `Fib<5>` will append `Fib<3>` and `Fib<4>` to the list
> * then we visit `Fib<3>` and append `Fib<1>` and `Fib<2>` to the list
> * then we visit `Fib<4>` and add no new entries
> 
> We pass declarations to the consumer when we're done with the instantiation 
> step. *Sometimes* this includes instantiating variables referenced by that 
> variable, and *sometimes* it doesn't. The difference is whether we're 
> performing "recursive" instantiation or not.
> 
> When we're performing immediate instantiation of a variable (either because 
> it was explicitly instantiated, or because we might need its value 
> immediately because it might be usable in constant expressions), our 
> instantiation step is non-recursive. We just add declarations to `Sema`'s "to 
> be instantiated at end of TU" list. This is at least a little important 
> semantically: we allow a matching specialization to be declared after the 
> first use wherever possible. In that case, we'll pass a declaration to the 
> consumer before we've instantiated the things it references.
> 
> When we're performing the end-of-TU instantiation of all referenced template 
> 

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563
+  } else if (IsInstantiation ||
  getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
  D->hasAttr()) {

rnk wrote:
> @rsmith , if inline global variable initialization has ordering requirements, 
> we have a bug, because I believe this GVA_DiscardableODR codepath handles 
> them, and we come through here to give them separate initializers in 
> llvm.global_ctors. See the example with two separate global_ctors entries on 
> godbolt:
> https://gcc.godbolt.org/z/5d577snqb
> 
> As long as LLVM doesn't provide ordering guarantees about same priority 
> initializers in global_ctors, inline globals have the same problems as 
> template instantiations. IMO whatever solution we use to order inline globals 
> should be used for template instantiations.
> 
> Intuitively, that means LLVM should promise to run global_ctors in 
> left-to-right order, and if all TUs instantiate initializers in the same 
> order, everything should behave intuitively.
> 
> The question then becomes, why doesn't this work already?
> Intuitively, that means LLVM should promise to run global_ctors in 
> left-to-right order

I don't think that's sufficient, due to the way we use COMDATs to discard 
duplicate global initializers. Consider:

TU 1 defines inline variable A.
TU 2 defines inline variable A and then inline variable B.
The standard guarantees that A is initialized before B in this scenario. But if 
(somehow) the linker picks the definition of A from TU 2, but orders the 
initializers from TU 1 first, then the resulting global_ctors order will be B 
then A, which is not allowed.

Either we need a guarantee that linkers will use the same ordering between 
objects when picking COMDATs as when concatenating `.init_array`, or we need to 
stop using the COMDAT trick for them (and either make LLVM respect the 
`@llvm.global_ctors` order or coalesce all inline variable initializers into 
the same function we run non-inline initializers from or something). Getting 
that guarantee seems like the best path to me, since the other option will 
presumably mean we check the guard variable once on startup for each TU that 
defines the variable, and it's something I expect linkers already happen to 
guarantee in practice.

> IMO whatever solution we use to order inline globals should be used for 
> template instantiations.

That sounds like it would regress what globalopt is able to optimize, for no 
gain in conformance nor perhaps any real gain in initialization order 
guarantees. The inline variable case is different, both because we can 
guarantee an initialization order and because the standard requires us to do 
so. Should we add complexity to the compiler whose only purpose is to mask 
bugs, if that complexity doesn't actually define away the possibility of bad 
behavior?

If we can actually describe a rule that we provide for initialization order of 
instantiated variables, and we can easily implement that rule and be confident 
we won't want to substantially weaken it later, and we can thereby assure our 
users that we will satisfy that rule, then I think that could be interesting, 
but anything less than that doesn't seem worthwhile to me.

> The question then becomes, why doesn't this work already?

It looks like it mostly does.

One factor here is instantiation order. When we instantiate a variable, we add 
the variables that it references to our "to be instantiated" list, which is 
processed later:
```
template int Fib = Fib + Fib;
template<> int Fib<0> = 0;
template<> int Fib<1> = 1;
```
* instantiating `Fib<5>` will append `Fib<3>` and `Fib<4>` to the list
* then we visit `Fib<3>` and append `Fib<1>` and `Fib<2>` to the list
* then we visit `Fib<4>` and add no new entries

We pass declarations to the consumer when we're done with the instantiation 
step. *Sometimes* this includes instantiating variables referenced by that 
variable, and *sometimes* it doesn't. The difference is whether we're 
performing "recursive" instantiation or not.

When we're performing immediate instantiation of a variable (either because it 
was explicitly instantiated, or because we might need its value immediately 
because it might be usable in constant expressions), our instantiation step is 
non-recursive. We just add declarations to `Sema`'s "to be instantiated at end 
of TU" list. This is at least a little important semantically: we allow a 
matching specialization to be declared after the first use wherever possible. 
In that case, we'll pass a declaration to the consumer before we've 
instantiated the things it references.

When we're performing the end-of-TU instantiation of all referenced template 
specializations, we do that recursively, and that means that we will 
instantiate any referenced variables *before* we pass the referencing variable 
to the AST consumer.

You can see this happening here: 

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563
+  } else if (IsInstantiation ||
  getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR ||
  D->hasAttr()) {

@rsmith , if inline global variable initialization has ordering requirements, 
we have a bug, because I believe this GVA_DiscardableODR codepath handles them, 
and we come through here to give them separate initializers in 
llvm.global_ctors. See the example with two separate global_ctors entries on 
godbolt:
https://gcc.godbolt.org/z/5d577snqb

As long as LLVM doesn't provide ordering guarantees about same priority 
initializers in global_ctors, inline globals have the same problems as template 
instantiations. IMO whatever solution we use to order inline globals should be 
used for template instantiations.

Intuitively, that means LLVM should promise to run global_ctors in 
left-to-right order, and if all TUs instantiate initializers in the same order, 
everything should behave intuitively.

The question then becomes, why doesn't this work already?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

> The underlying problem is basically wg21.link/cwg362 which has no concensus 
> yet.

CWG362 has a clear consensus and has been closed with that consensus for 18 
years. The consensus, per that issue, is:

> We discussed this and agreed that **we really do mean the the order is 
> unspecified**.

which seems clear that our current behavior is a valid choice.

In D126341#3537947 , @rnk wrote:

> First, why should these guarantees be limited to instantiations and not 
> inline variables? Such as:
>
>   int f();
>   inline int gv1 = f();
>   inline int gv2 = gv1 + 1; // rely on previous

Inline variables have initialization order guarantees already. An inline 
variable `B` can rely on a prior inline variable `A` being initialized first if 
`A` is defined before `B` in every TU where `B` is defined. And a non-inline 
variable `C` can rely on a prior inline variable `B` being initialized first if 
`B` is defined before `C`.

There is no comparable guarantee for instantiated variables, in part because an 
intended implementation strategy is to instantiate them separately, and combine 
those instantiation units with the compilation units at link time in an 
arbitrary order, and in part because instantiation order could in general be 
different in different TUs.

> Second, LLVM doesn't guarantee that global_ctors at the same priority execute 
> in order. See the langref: 
> https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable So, 
> without a guarantee from LLVM, Clang can't rely on this behavior. LLVM relies 
> on this lack of an ordering guarantee to power globalopt.

It doesn't seem reasonable for us to provide a guarantee here, and in fact, we 
//can't// provide a guarantee in general (once there's more than one TU, a 
consistent global total order might not exist). I think the question is, should 
we make some minor effort to make the broken code that's relying on 
initialization order of unordered variables happen to do the right thing most 
of the time, or should we keep the status quo that is likely to result in 
problems happening earlier? I don't think it's clear whether the superficial 
increase in compatibility with GCC is a net positive or negative -- giving the 
surprising initialization order may counter-intuitively help people to find 
ordering bugs faster.




Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:582
 
-AddGlobalCtor(Fn, 65535, COMDATKey);
+AddGlobalCtor(Fn, 65535, COMDATKey, IsInstantiation);
 if (COMDATKey && (getTriple().isOSBinFormatELF() ||

This is effectively initializing instantiated variables in reverse 
instantiation order. That seems like it'll make things worse as much as it 
makes things better. For example, given:

```
#include 
template int a = (std::cout << "hello, ", 0);
template int b = (std::cout << "world", 0);
int main() {
  (void)a;
  (void)b;
}
```

... we currently print `"hello, world"`, but with this change we'll print 
`"worldhello, "`.

If we want a sensible initialization order, I think we need a different 
strategy, that will probably require `Sema` to be a lot more careful about what 
order it instantiates variables in and what order it passes them to the AST 
consumer: if an instantiation A triggers another instantiation B, we should 
defer passing A to the consumer until B has been instantiated and passed to the 
consumer. That's probably not too hard to implement, by adding an entry to the 
pending instantiation list to say "now pass this to the consumer" in the case 
where one instantiation triggers another. But I do wonder whether that level of 
complexity is worthwhile, given that code relying on this behavior is broken.



Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1562-1563
   // FIXME: Type coercion of void()* types.
-  GlobalCtors.push_back(Structor(Priority, Ctor, AssociatedData));
+  if (InsertFront)
+GlobalCtors.emplace(GlobalCtors.begin(), Priority, Ctor, AssociatedData);
+  else

This is quadratic in the number of instantiated variables. I don't think that's 
acceptable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D126341#3537947 , @rnk wrote:

> I'm somewhat supportive of the goal here, but I think there are still some 
> underlying issues.
>
> First, why should these guarantees be limited to instantiations and not 
> inline variables? Such as:
>
>   int f();
>   inline int gv1 = f();
>   inline int gv2 = gv1 + 1; // rely on previous

I think this is because (as discussed in cwg362, 
https://eel.is/c++draft/lex.phases#1.8) instantiations happen in instantiation 
units where no order is specified. inline variables live in TU, where the order 
is specified.

> Second, LLVM doesn't guarantee that global_ctors at the same priority execute 
> in order. See the langref: 
> https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable So, 
> without a guarantee from LLVM, Clang can't rely on this behavior. LLVM relies 
> on this lack of an ordering guarantee to power globalopt.

Yeah, I noticed that too. In practice, it looks like we do rely on the order in 
llvm.global_ctors to implement the static init order (for the above inline 
variable case, https://clang.godbolt.org/z/G3YoY5bc9). If globalopt decides to 
reorder the two init functions, the result would be wrong.

> Last, what happens when the same global is implicitly instantiated in some 
> other TU? Won't that disrupt the ordering?
>
> +@alexander-shaposhnikov, who is working on global opt changes.

Hmm, that's a great point. My thoughts are it would not unless the linker is 
weird/inconsistent about which COMDAT to pick. Since for each TU, one globalvar 
has all its dependency globalvars initialized in the correct order in init 
array. Say two TUs, both have T<8>, no matter which T<8> the linker picks, it 
would pick all the T<8>'s dependencies according to the same criteria. Hence 
the init order is kept. I wouldn't say I'm 100% sure. At least it appears to me 
so at the moment. I'll do some experiments locally.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a subscriber: alexander-shaposhnikov.
rnk added a comment.

I'm somewhat supportive of the goal here, but I think there are still some 
underlying issues.

First, why should these guarantees be limited to instantiations and not inline 
variables? Such as:

  int f();
  inline int gv1 = f();
  inline int gv2 = gv1 + 1; // rely on previous

Second, LLVM doesn't guarantee that global_ctors at the same priority execute 
in order. See the langref: 
https://llvm.org/docs/LangRef.html#the-llvm-global-ctors-global-variable So, 
without a guarantee from LLVM, Clang can't rely on this behavior. LLVM relies 
on this lack of an ordering guarantee to power globalopt.

Last, what happens when the same global is implicitly instantiated in some 
other TU? Won't that disrupt the ordering?

+@alexander-shaposhnikov, who is working on global opt changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D126341#3537675 , @aaron.ballman 
wrote:

> Adding the language WG as a reviewer in case others have opinions.
>
>> The underlying problem is basically wg21.link/cwg362 which has no concensus 
>> yet.
>
> According to 
> https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#362 this was 
> resolved in CD1 and we track it as being not applicable to us 
> (https://clang.llvm.org/cxx_dr_status.html#362). (Is our status actually 
> correct for this?)
>
>> Will the reviewers be supportive if I make the original cwg362 test case 
>> work too?
>
> To me, it depends on what it does to compile times and memory overhead for 
> the compiler when run on large projects. If the extra tracking is cheap and 
> doesn't really impact anything, I think it's reasonable to want to match 
> GCC's behavior. If it turns out this is expensive, I'm less keen on matching 
> GCC.

Thanks for the opinion @aaron.ballman. I think the cost would be low. I'll get 
back with some numbers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: clang-language-wg.
aaron.ballman added a comment.

Adding the language WG as a reviewer in case others have opinions.

> The underlying problem is basically wg21.link/cwg362 which has no concensus 
> yet.

According to https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#362 
this was resolved in CD1 and we track it as being not applicable to us 
(https://clang.llvm.org/cxx_dr_status.html#362). (Is our status actually 
correct for this?)

> Will the reviewers be supportive if I make the original cwg362 test case work 
> too?

To me, it depends on what it does to compile times and memory overhead for the 
compiler when run on large projects. If the extra tracking is cheap and doesn't 
really impact anything, I think it's reasonable to want to match GCC's 
behavior. If it turns out this is expensive, I'm less keen on matching GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Yuanfang Chen via Phabricator via cfe-commits]

ychen updated this revision to Diff 431825.
ychen added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126341

Files:
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/test/CodeGenCXX/aix-static-init-temp-spec-and-inline-var.cpp
  clang/test/CodeGenCXX/microsoft-abi-static-initializers.cpp
  clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
  clang/test/Modules/initializers.cpp

Index: clang/test/Modules/initializers.cpp
===
--- clang/test/Modules/initializers.cpp
+++ clang/test/Modules/initializers.cpp
@@ -154,12 +154,12 @@
 
 // It's OK if the order of the first 6 of these changes.
 // CHECK: @llvm.global_ctors = appending global
-// CHECK-SAME: @[[E_INIT:[^,]*]], {{[^@]*}} @[[E]]
-// CHECK-SAME: @[[F_INIT:[^,]*]], {{[^@]*}} @[[F]]
-// CHECK-SAME: @[[XA_INIT:[^,]*]], {{[^@]*}} @[[XA]]
-// CHECK-SAME: @[[XE_INIT:[^,]*]], {{[^@]*}} @[[XE]]
-// CHECK-SAME: @[[XF_INIT:[^,]*]], {{[^@]*}} @[[XF]]
 // CHECK-SAME: @[[XB_INIT:[^,]*]], {{[^@]*}} @[[XB]]
+// CHECK-SAME: @[[XF_INIT:[^,]*]], {{[^@]*}} @[[XF]]
+// CHECK-SAME: @[[XE_INIT:[^,]*]], {{[^@]*}} @[[XE]]
+// CHECK-SAME: @[[XA_INIT:[^,]*]], {{[^@]*}} @[[XA]]
+// CHECK-SAME: @[[F_INIT:[^,]*]], {{[^@]*}} @[[F]]
+// CHECK-SAME: @[[E_INIT:[^,]*]], {{[^@]*}} @[[E]]
 // CHECK-IMPORT-SAME: @[[TU_INIT:[^,]*]], i8* null }]
 
 // FIXME: Should this use __cxa_guard_acquire?
Index: clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
@@ -29,32 +29,41 @@
 // ALL: @_ZN1AIbE1aE ={{.*}} global i32 10
 template<> int A::a = 10;
 
-// ALL: @llvm.global_ctors = appending global [8 x { i32, void ()*, i8* }]
+// ALL: @llvm.global_ctors = appending global [11 x { i32, void ()*, i8* }]
 
-// ELF: [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered1:[^,]*]], i8* bitcast (i32* @_ZN1AIsE1aE to i8*) },
-// MACHO: [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered1:[^,]*]], i8* null },
+// ELF:  [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered10:[^,]*]], i8* bitcast (i32* @_ZN1RILi1EE1aE to i8*) },
+// MACHO: [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered10:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered2:[^,]*]], i8* bitcast (i16* @_Z1xIsE to i8*) },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered2:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered9:[^,]*]], i8* bitcast (i32* @_ZN1RILi2EE1aE to i8*) },
+// MACHO: { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered9:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered3:[^,]*]], i8* bitcast (i32* @_ZN2ns1aIiE1iE to i8*) },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered3:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered8:[^,]*]], i8* bitcast (i32* @_ZN1RILi3EE1aE to i8*) },
+// MACHO: { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered8:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered4:[^,]*]], i8* bitcast (i32* @_ZN2ns1b1iIiEE to i8*) },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered4:[^,]*]], i8* null },
+// ALL:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered7:[^,]*]], i8* null },
+
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* @_Z1xIcE },
+// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* null },
 
 // ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered5:[^,]*]], i8* bitcast (i32* @_ZN1AIvE1aE to i8*) },
 // MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered5:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* @_Z1xIcE },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered4:[^,]*]], i8* bitcast (i32* @_ZN2ns1b1iIiEE to i8*) },
+// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered4:[^,]*]], i8* null },
 
-// ALL:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered7:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered3:[^,]*]], i8* bitcast (i32* @_ZN2ns1aIiE1iE to i8*) },
+// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered3:[^,]*]], i8* null },
+
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered2:[^,]*]], i8* bitcast (i16* @_Z1xIsE to i8*) },
+// MACHO:  { i32, void ()*, i8* } { 

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

[Yuanfang Chen via Phabricator via cfe-commits]

ychen created this revision.
ychen added reviewers: rnk, rsmith, aaron.ballman.
Herald added a project: All.
ychen requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

By the standard https://eel.is/c++draft/basic.start#dynamic-1, implicitly 
instantiated global variable's initializer has no order. However GCC has 
the *intuitive behavior* for the two test cases in 
https://clang.godbolt.org/z/MPdhYTqhK.

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

I wish both cases could work, like GCC. However, to make the original cwg362 
test case work,
I needs an extra data structure to track the instantiation order for implicitly 
instantiated global variable. So the current patch only work for the first test 
case.

Will the reviewers be supportive if I make the original cwg362 test case work 
too?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126341

Files:
  clang/test/CodeGenCXX/aix-static-init-temp-spec-and-inline-var.cpp
  clang/test/CodeGenCXX/microsoft-abi-static-initializers.cpp
  clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
  clang/test/Modules/initializers.cpp

Index: clang/test/Modules/initializers.cpp
===
--- clang/test/Modules/initializers.cpp
+++ clang/test/Modules/initializers.cpp
@@ -154,12 +154,12 @@
 
 // It's OK if the order of the first 6 of these changes.
 // CHECK: @llvm.global_ctors = appending global
-// CHECK-SAME: @[[E_INIT:[^,]*]], {{[^@]*}} @[[E]]
-// CHECK-SAME: @[[F_INIT:[^,]*]], {{[^@]*}} @[[F]]
-// CHECK-SAME: @[[XA_INIT:[^,]*]], {{[^@]*}} @[[XA]]
-// CHECK-SAME: @[[XE_INIT:[^,]*]], {{[^@]*}} @[[XE]]
-// CHECK-SAME: @[[XF_INIT:[^,]*]], {{[^@]*}} @[[XF]]
 // CHECK-SAME: @[[XB_INIT:[^,]*]], {{[^@]*}} @[[XB]]
+// CHECK-SAME: @[[XF_INIT:[^,]*]], {{[^@]*}} @[[XF]]
+// CHECK-SAME: @[[XE_INIT:[^,]*]], {{[^@]*}} @[[XE]]
+// CHECK-SAME: @[[XA_INIT:[^,]*]], {{[^@]*}} @[[XA]]
+// CHECK-SAME: @[[F_INIT:[^,]*]], {{[^@]*}} @[[F]]
+// CHECK-SAME: @[[E_INIT:[^,]*]], {{[^@]*}} @[[E]]
 // CHECK-IMPORT-SAME: @[[TU_INIT:[^,]*]], i8* null }]
 
 // FIXME: Should this use __cxa_guard_acquire?
Index: clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
===
--- clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
+++ clang/test/CodeGenCXX/static-member-variable-explicit-specialization.cpp
@@ -29,32 +29,41 @@
 // ALL: @_ZN1AIbE1aE ={{.*}} global i32 10
 template<> int A::a = 10;
 
-// ALL: @llvm.global_ctors = appending global [8 x { i32, void ()*, i8* }]
+// ALL: @llvm.global_ctors = appending global [11 x { i32, void ()*, i8* }]
 
-// ELF: [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered1:[^,]*]], i8* bitcast (i32* @_ZN1AIsE1aE to i8*) },
-// MACHO: [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered1:[^,]*]], i8* null },
+// ELF:  [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered10:[^,]*]], i8* bitcast (i32* @_ZN1RILi1EE1aE to i8*) },
+// MACHO: [{ i32, void ()*, i8* } { i32 65535, void ()* @[[unordered10:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered2:[^,]*]], i8* bitcast (i16* @_Z1xIsE to i8*) },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered2:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered9:[^,]*]], i8* bitcast (i32* @_ZN1RILi2EE1aE to i8*) },
+// MACHO: { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered9:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered3:[^,]*]], i8* bitcast (i32* @_ZN2ns1aIiE1iE to i8*) },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered3:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered8:[^,]*]], i8* bitcast (i32* @_ZN1RILi3EE1aE to i8*) },
+// MACHO: { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered8:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered4:[^,]*]], i8* bitcast (i32* @_ZN2ns1b1iIiEE to i8*) },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered4:[^,]*]], i8* null },
+// ALL:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered7:[^,]*]], i8* null },
+
+// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* @_Z1xIcE },
+// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* null },
 
 // ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered5:[^,]*]], i8* bitcast (i32* @_ZN1AIvE1aE to i8*) },
 // MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered5:[^,]*]], i8* null },
 
-// ELF:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* @_Z1xIcE },
-// MACHO:  { i32, void ()*, i8* } { i32 65535, void ()* @[[unordered6:[^,]*]], i8* null },
+// ELF:  { i32, void ()*, i8* } {