JonChesterfield marked 4 inline comments as done. JonChesterfield added inline comments.
================ Comment at: clang/include/clang/Basic/AttrDocs.td:4367 +This is useful for variables that are always written to before use where the +default zero initialization provided by the toolchain loader is expensive. + }]; ---------------- rjmccall wrote: > How about: > > > The ``loader_uninitialized`` attribute can be placed on global variables to > > indicate that the variable does not need to be zero initialized by the > > loader. > > On most targets, zero-initialization does not incur any additional cost. > > For example, most general purpose operating systems deliberately ensure > > that all memory is properly initialized in order to avoid leaking privileged > > information from the kernel or other programs. However, some targets > > do not make this guarantee, and on these targets, avoiding an unnecessary > > zero-initialization can have a significant impact on load times and/or code > > size. > > > > A declaration with this attribute is a non-tentative definition just as if > > it > > provided an initializer. Variables with this attribute are considered to > > be > > uninitialized in the same sense as a local variable, and the programs must > > write to them before reading from them. If the variable's type is a C++ > > class > > type with a non-trivial default constructor, or an array thereof, this > > attribute > > only suppresses the static zero-initialization of the variable, not the > > dynamic > > initialization provided by executing the default constructor. That's a lot better! Thank you. Updated the patch to use your wording verbatim. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7436 + case ParsedAttr::AT_LoaderUninitialized: + handleLoaderUninitializedAttr(S, D, AL); + break; ---------------- aaron.ballman wrote: > rjmccall wrote: > > JonChesterfield wrote: > > > aaron.ballman wrote: > > > > If you don't need any custom semantic checking, you can remove that > > > > function and instead call > > > > `handleSimpleAttribute<LoaderUninitializedAttr>(S, D, AL);` > > > I think this patch does need some custom semantic checking, I just > > > haven't been able to work out how to implement it. Specifically, the > > > attribute is an initializer, so > > > > > > `int foo __attribute__((loader_uninitialised)) = some_value;` > > > > > > should be a warning, as the = some_value is going to be ignored. > > This should be an error, not a warning, unless there's a specific need to > > be lenient. > Agreed that this should be an error rather than a warning. > > Not 100% certain, but I suspect you'll need to add that checking to > `Sema::AddInitializerToDecl()` because I'm guessing that the initializer has > not been processed by the time the attributes are being applied to the > variable declaration. You can check `VarDecl::hasInit()` within > `handleLoaderUninitializedAttr()` to see if that specific declaration has an > initializer, but I'm betting that gives you the wrong answer. Nice, Yes, that's much better - I should have searched for the opencl `__local` handling. As you suspected, hasInit() just returns true at that point. ================ Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + ---------------- Quuxplusone wrote: > This test case is identical to line 36 of > clang/test/Sema/attr-loader-uninitialized.cpp, where you say you don't want > it to compile at all. > > I think you need a clearer idea of how this interacts with initializers. Is > it merely supposed to eliminate the //zero-initialization// that happens > before the user-specified construction/initialization, or is it supposed to > compete with the user-specified construction/initialization? > > That is, for > > nontrivial unt [[clang::loader_uninitialized]]; > > is it merely supposed to call `unt::unt()` on a chunk of undef memory > (instead of the usual chunk of zeroed memory), or is it supposed to skip the > constructor entirely? And for > > int x [[clang::loader_uninitialized]] = foo(); > > is it merely supposed to call `foo()` and assign the result to a chunk of > undef memory (instead of the usual chunk of zeroed memory), or is it supposed > to skip the initialization entirely? I think you commented while the first working piece of sema landed. My thinking is relatively clear but my understanding of clang's semantic analysis is a work in progress! Initializers (`= foo()`) are straightforward. Error on the basis that the attribute effectively means `= undef`, and one should not have two initializers. A test case is now added for that (and now passes). The codegen I want for a default constructed global marked with this variable is: - global variable allocated, with undef as the original value - default constructor call synthesized - said default constructor set up for invocation from crt, before main, writing over the undef value Where the default constructor can be optimized as usual, e.g. if it always writes a constant, we can init with that constant instead of the undef and elide the constructor. I don't have that actually working yet - the constructor call is not being emitted, so we just have the undef global. I think it's important to distinguish between the values of the bits when the program is loaded and whether constructor/destructors are called, as one could want any combination of the two. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
