rnk added a subscriber: asbirlea.
rnk added a comment.
I think we do need to think about globalopt. Consider this example:
extern "C" {
int printf(const char *, ...);
extern int sharedState;
int sharedState = 0;
int init1() {
asm volatile(""); // block globalopt
return ++sharedState;
}
int init2() { return ++sharedState; }
int gv1 = init1();
int gv2 = init2();
}
int main() {
printf("gv1: %d\n", gv1);
printf("gv2: %d\n", gv2);
}
These initializers are ordered, and gv1 should be 1 and gv2 should be 2. Clang
produces a single shared initializer `@_GLOBAL__sub_I_${src}` to ensure this
ordering. However, if you manually split the initializers apart
(https://reviews.llvm.org/P8266) and try to rely on the new langref ordering,
globalopt will fold the second initializer and it appears as if `init2` ran
first. I have these two files in t.cpp and t.ll, and see the results:
$ clang -O2 t.cpp && ./a.out
gv1: 1
gv2: 2
$ clang -O2 t.ll && ./a.out
gv1: 2
gv2: 1
I've never been totally comfortable with the soundness of globalopt, so I
wonder if that's the part that really needs to change. The value of globalopt
is that it can fold simple constructors, and those don't access other mutable
global variables. If we had some way of telling globalopt that it is only
allowed to read from constants and the GV being initialized, that would solve
this issue. Right now, there is no link in the IR between a global and its
initializer unless the global is comdat.
---
As for this patch, I think we're not ready to add the LangRef guarantee.
However, the code change is good: it ensures that -f[no-]use-init-array doesn't
needlessly reverse the order of initialization in the TU.
Without the langref guarantee, clang is skating on thin ice: inline static data
member initializers may not be run in source order. However, globalopt is
currently completely unable to fold initializers for weak/comdat globals, so in
practice, there is no risk to users.
I think we can defer the langref change until the next time someone makes a
serious investment in globalopt. I think @asbirlea and @aeubanks might be
interested in that sometime later this year.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103495/new/
https://reviews.llvm.org/D103495
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
