Handling of constant initializers needs cleaning

[llvmbot]

Bugzilla Link	3521
Resolution	FIXED
Resolved on	Mar 12, 2010 00:57
Version	unspecified
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@DougGregor,@efriedma-quic

Extended Description

Sema currently has some extra code for accepting static initializers which is not present in Evaluate. As a consequence, isConstantInitializer returns false for things which Sema has accepted as constant initializers.

This causes problems for CodeGen, which should not have to replicate this code.

For example, replacing calls to CheckConstantInitializer with:

if (!CheckForConstantInitializer(Init, DclT))
assert(Init->isConstantInitializer(Context) && "Invalid CheckForConstantInitializer result.");

results in 10 assertions just when running the clang test suite.

We need a number of cleanups here:

1. Decl's should distinguish between constant and non-constant initializers (C++).

2. The special Sema code for accepting more things as constant initializers needs to be killed off and moved into Evaluate.

3. CodeGen should stop doing any more work than is necessary to emit constant initializers. The constant emission should only worry about walking the initializer tree, but not about actually constructing values for things; it should rely on Evaluate for that.

This bug is a placeholder for this work.

[efriedma-quic]

For example, replacing calls to CheckConstantInitializer with:

if (!CheckForConstantInitializer(Init, DclT))
assert(Init->isConstantInitializer(Context) && "Invalid
CheckForConstantInitializer result.");

results in 10 assertions just when running the clang test suite.

I think the vast majority of those are bug 3398. We definitely want to get that working, though.

We need a number of cleanups here:

1. Decl's should distinguish between constant and non-constant initializers
   (C++).

Is there some particular reason we need this? Besides ICEs, as far as I know, C++ doesn't have a concept of constant initializers.

2. The special Sema code for accepting more things as constant initializers
   needs to be killed off and moved into Evaluate.

Well, sort of... we want Evaluate to be able to handle everything, and then make the Sema code strict enough to warn on questionable cases in -pedantic mode.

3. CodeGen should stop doing any more work than is necessary to emit constant
   initializers. The constant emission should only worry about walking the
   initializer tree, but not about actually constructing values for things; it
   should rely on Evaluate for that.

There really isn't much to eliminate, unless you're considering making Evaluate handle aggregates...

[lattner]

I think that the right approach is really to extend Evaluate to support these cases in a common framework. Evaluate is really about "constant fold me as hard as possible", and it should handle these cases.

[llvmbot]

For example, replacing calls to CheckConstantInitializer with:

if (!CheckForConstantInitializer(Init, DclT))
assert(Init->isConstantInitializer(Context) && "Invalid
CheckForConstantInitializer result.");

results in 10 assertions just when running the clang test suite.

I think the vast majority of those are bug 3398. We definitely want to get
that working, though.

Yes. Bug 3398 is just a meta bug though, it would go away if Evaluate knew
how to fold everything we accept as a constant initializer.

We need a number of cleanups here:

1. Decl's should distinguish between constant and non-constant initializers
   (C++).

Is there some particular reason we need this? Besides ICEs, as far as I know,
C++ doesn't have a concept of constant initializers.

Mostly for cleanliness. They are very different cases in the AST, so we should
disambiguate them I think. Although may be it doesn't make sense, outside C++
mode clients know that static initializers are constants.

2. The special Sema code for accepting more things as constant initializers
   needs to be killed off and moved into Evaluate.

Well, sort of... we want Evaluate to be able to handle everything, and then
make the Sema code strict enough to warn on questionable cases in -pedantic
mode.

Right. I think Evaluate has all the infrastructure for this though, we just need
to handle the extra cases with appropriate feedback?

3. CodeGen should stop doing any more work than is necessary to emit constant
   initializers. The constant emission should only worry about walking the
   initializer tree, but not about actually constructing values for things; it
   should rely on Evaluate for that.

There really isn't much to eliminate, unless you're considering making Evaluate
handle aggregates...

Hmm, you're right! :) The casting code is I guess what I was thinking about.

[llvmbot]

As a first step on this, I am planning to change IRgen's constant emission to be able to fail; i.e. EmitConstantExpr can return 0. I think we will want this for C++, since we should try to emit initializers as constants before falling back to guarded initializers.

[llvmbot]

As of: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090216/012802.html

Evaluate() handles the main extended case "(int) &someglobal". Still need cleanups (removing Sema & IRgen code).

[efriedma-quic]

With Daniel's work on allowing pointers cast to integers in constant expressions and a few small patches I just committed, the number of cases in the testsuite which isConstantInitializer rejects but CheckForConstantInitializer accepts is reduced to a few cases. The tests in question are test/Sema/const-ptr-int-ptr-cast.c, test/Sema/static-init.c, test/SemaObjC/comptypes-8.m, and test/SemaObjC/protocol-atttribute-1.m.

The two missing cases in test/Sema are due to casting pointers to "long long". I'm not sure if we really want/need to accept this.

The two missing cases in test/SemaObjC are due to initializing ObjC id types with the integer 0; I don't really want to touch this because I'm not too familiar with ObjC, but it shouldn't be too hard to fix.

[llvmbot]

Cool; thanks for the investigation. I think the main thing to do now is to kill off the extra Sema code which accepts those things (adding support to Evaluate or changing tests depending on if we think we care).

[efriedma-quic]

Is there anything else left to do here?

[llvmbot]

I don't think so; at least we don't need the bug anymore. Thanks!