This is an archive of the discontinued LLVM Phabricator instance.

Add option -fkeep-static-consts
ClosedPublic

Authored by eandrews on Dec 6 2017, 3:24 PM.

Details

Summary

Currently clang does not emit unused static constants. GCC emits these constants by default when there is no optimization.

GCC 's option -fkeep-static-consts seems to be buggy. I am not sure what exactly it does because default behavior emits unused static constants (i.e. without the use of any option) and this option does not work with optimization. Its negation can be used to not emit the static constants.

In Clang, since default behavior does not keep unused static constants, I implemented -fkeep-static-consts to emit these if required. This could be useful for producing identification strings like SVN identifiers inside the object file even though the string isn't used by the program.

Diff Detail

Repository
rC Clang

Event Timeline

eandrews created this revision.Dec 6 2017, 3:24 PM
rnk added a comment.Dec 14 2017, 1:40 PM

This isn't sufficient, GlobalDCE will remove the internal constant. It's also unlikely that the constant will survive --gc-sections / -fdata-sections. A better solution would be to add a new attribute (__attribute__((nondiscardable))? too close to nodiscard?) that adds the global in question to @llvm.used and excludes it from -fdata-sections.

Thanks for the review Reid. Sorry for the delay in my response. I was OOO.

I am not sure if a new attribute is necessary. __ attribute __(used) is already supported in Clang. While this attribute can be used to retain static constants, it would require the user to modify the source which may not always be possible/practical. Its also interesting to note that GCC actually retains unused static constants by default. fno-keep-static-consts is used to remove unused static constants in GCC.

rnk added a comment.Jan 17 2018, 2:42 PM

OK. My concern is that users need to use __attribute__((used)) or something more robust if they want SVN identifiers to reliably appear in the output. Adding this flag just creates a trap that will fail once they turn on -O2. I'd rather not have it in the interface to avoid that user confusion.

That makes sense. Is it not possible to implement the required functionality using a flag vs an attribute? In an earlier comment you mentioned adding the global to @llvm.used to prevent GlobalDCE from removing it. Can you not do that when using a flag?

OK. My concern is that users need to use attribute((used)) or something more robust if they want SVN identifiers to reliably appear in the output. Adding this flag just creates a trap that will fail once they turn on >>-O2. I'd rather not have it in the interface to avoid that user confusion.

Since the idea here is to have a global option for 'always emit these things', could a viable solution simply add all of these things to @llvm.used. This wouldn't affect the usefulness of __attribute((used)), but could be implemented similarly, right?

I think we can use CodeGenModule::addUsedGlobal for this purpose. I noticed this is what attribute 'used' calls. I need to look into it though.

eandrews updated this revision to Diff 161307.Aug 17 2018, 1:34 PM
eandrews edited the summary of this revision. (Show Details)

This patch fell through the cracks earlier. I apologize. Based on Reid's and Erich's feedback, I am now adding the variable to @llvm.used. Additionally I modified the if statements to ensure only static variables are considered.

I also compared the IR when using the option -fkeep-static-consts while compiling the program, vs adding attribute((used)) to individual variables inside the program. The generated IR had no difference. I assume this means all required information to prevent the variable from being removed in later stages are now being emitted?

rnk added inline comments.Aug 17 2018, 4:27 PM
include/clang/Basic/LangOptions.def
311 ↗(On Diff #161307)

Let's make this a CodeGenOption, since only CodeGen needs to look at it.

eandrews updated this revision to Diff 161544.Aug 20 2018, 12:54 PM

Based on Reid's feedback, I changed option to CodeGenOption

eandrews marked an inline comment as done.Aug 20 2018, 12:55 PM
eandrews added inline comments.
include/clang/Basic/LangOptions.def
311 ↗(On Diff #161307)

Thanks for the feedback Reid! I've changed it

rnk accepted this revision.Aug 20 2018, 1:08 PM

lgtm!

This revision is now accepted and ready to land.Aug 20 2018, 1:08 PM
eandrews marked an inline comment as done.Aug 20 2018, 1:18 PM
In D40925#1206416, @rnk wrote:

lgtm!

Thanks!

This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: zturner.Oct 25 2018, 10:45 AM

My coworker, @zturner, asked if I knew a way to force emit all constants, and I mentioned this flag, but we noticed it doesn't work on cases when the constant is implicitly static, like this:

const int foo = 42;

If I add static to it, it gets emitted with -fkeep-static-consts, but as is, it doesn't get emitted.

Do you think it's worth extending the logic to handle this case? GCC seems to that global foo regardless of the flag setting.

In D40925#1276114, @rnk wrote:

My coworker, @zturner, asked if I knew a way to force emit all constants, and I mentioned this flag, but we noticed it doesn't work on cases when the constant is implicitly static, like this:

const int foo = 42;

If I add static to it, it gets emitted with -fkeep-static-consts, but as is, it doesn't get emitted.

Do you think it's worth extending the logic to handle this case? GCC seems to that global foo regardless of the flag setting.

I looked into it real quick, and think that checking storage class instead of duration is incorrect here. See https://reviews.llvm.org/D53718 for the fix @zturner is asking for (I think!).

I think it makes sense to include this case just to make the flag more complete. Also, we don't really match GCC behavior for this flag. GCC emits all static constants by default (when O0). When optimized, this flag has no effect on GCC. The negation is used to not emit the constants.

Erich has uploaded a possible fix here - https://reviews.llvm.org/D53718