[PATCH] D74361: [Clang] Undef attribute for global variables

[Jon Chesterfield via Phabricator via cfe-commits]

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