llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.98k stars 11.94k forks source link

[RFE] add `-fno-lifetime-dse` option #40040

Open llvmbot opened 5 years ago

llvmbot commented 5 years ago
Bugzilla Link 40694
Version unspecified
OS Linux
Reporter LLVM Bugzilla Contributor
CC @DougGregor,@zygoloid

Extended Description

g++ has the -fno-lifetime-dse option to support the code that expects the value in memory not affected by placement new, when creating PODs. That is, you can initialize some array with the needed values, and then use placement new to create POD objects on that array, and they will be initialized from the pre-existing values. This is a useful extension and it would be nice if clang to also support it.

llvmbot commented 5 years ago

This extension solves one very specific problem: it avoids DSE before placement new. That is not meaningful to users, and it's not meaningful to us either -- it does not indicate which optimizations other than DSE (that we may or may not already implement) are valid before placement new. If meaningful user-level semantics cannot be given to this flag, then there's no way we can

Right, lets find out the better semantic. My suggestion is just to not extend its scope, i.e. not to deal with automatic storage or the destructors. If we agree to solve one small practical problem, things will be simpler.

I would just propose something like "volatile_placement_new" operator that would just guarantee the old value intact

Meaning what, exactly? If the old value were stored via a different type according to TBAA, can we reason about that?

Sure, they are stored by the different type, which is the main idea. This type is "char" (or unsigned char) that allows to alias anything, so I do not envision a big problems on the strict aliasing front. So I really think that only DSE is what can go wrong, which is why I think gcc did it the way it did.

If not, then I think what this gives you is "placement new over an object of the same type that is within its lifetime preserves the value of the old object". Does that match the intended semantic model?

No, I don't see a big need to replace the same type, at least not within the scope of this proposal.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 5 years ago

This extension solves one very specific problem: it avoids DSE before placement new.

That is not meaningful to users, and it's not meaningful to us either -- it does not indicate which optimizations other than DSE (that we may or may not already implement) are valid before placement new. If meaningful user-level semantics cannot be given to this flag, then there's no way we can reasonably implement it and provide any actual guarantees about what it does. If all it means is "turn off a specific optimization pass in GCC's optimizer", that's not something you should expect us to be able to honor.

I would just propose something like "volatile_placement_new" operator that would just guarantee the old value intact

Meaning what, exactly? If the old value were stored via a different type according to TBAA, can we reason about that? If not, then I think what this gives you is "placement new over an object of the same type that is within its lifetime preserves the value of the old object". Does that match the intended semantic model?

llvmbot commented 5 years ago

That is a terrible flag name :)

If we support this, we should provide a more reasonable name for the language extension we're supporting (perhaps -fno-strict-lifetime), along with documentation of exactly what guarantees we make in that mode. Eg: "do not assume that the lifetime of an object starts when its constructor runs and ends when its destructor runs; instead, permit the object to be accessed at any point within its storage duration." (Naturally, we'd want to allow -fno-lifetime-dse as a compatibility synonym.)

This extension solves one very specific problem: it avoids DSE before placement new. I understand you may want something more generic than that, but I'd like to disagree with that intention for the following reasons:

  1. Automatic storage is not a problem, as I don't think you have too many ways of pre-initializing it. The problem is very specific to placement new.
  2. Your example with destructor is synthetic. I think the only practical value is to care of the storage occupied by the placement new. And keep it after the object life-time is expired, is very simple: just don't call its destructor at all. I even set it to "=delete". Something you can hardly do with an automatic storage, and hardly ever need to.

So really, the only thing that is needed, is to avoid DSE before placement new, and that's why the name of an option is a bit cryptic. This extension can best be implemented on an optimization level, just like gcc already does. If I were to invent the standard proposal for it (I am not the right person to do that though), then even in that case I would just propose something like "volatile_placement_new" operator that would just guarantee the old value intact. This is enough to solve the problem, and has very few chances of breaking anything or add an inconsistency.

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 5 years ago

That is a terrible flag name :)

If we support this, we should provide a more reasonable name for the language extension we're supporting (perhaps -fno-strict-lifetime), along with documentation of exactly what guarantees we make in that mode. Eg: "do not assume that the lifetime of an object starts when its constructor runs and ends when its destructor runs; instead, permit the object to be accessed at any point within its storage duration." (Naturally, we'd want to allow -fno-lifetime-dse as a compatibility synonym.)

It's not clear exactly what this should mean for an automatic storage duration object, though. Consider:

void f(int);
struct A { int *p; ~A() { f(*p); } };
struct B { int n; };
void g() {
  A a;
  B b = { 123 };
  a.p = &b.n;
}

Should this be guaranteed to call f(123), or do we consider the storage for b to have disappeared before the destructor for a runs?