[libcxx-commits] [libcxx] [libcxx] Remove Redundant Reset in ~basic_string (PR #164718)

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> After thinking about this a bit more and talking to Reid, `dead_on_return` has more problems. It's important to note that adding `dead_on_return` to the destructor's this parameter also would not actually help here given inlining and how the pass pipeline is currently set up.`~basic_string` is likely to get inlined in many places, and after inlining `dead_on_return` goes away, so we have no additional information to guide AA/DSE. To fix that we would need to add an intrinsic similar to `@llvm.lifetime.end` to track that information. Or we would need to mark `__reset_internal_buffer()` as `always_inline` and add a EarlyDSE pass that takes care of trivial stores to handle this. Neither are great options, on top of what Reid already mentioned.

I'm not sure I follow. Adding `dead_on_return` seems to work just fine: https://godbolt.org/z/dv4oEq3eK. The IR is taken from this: https://godbolt.org/z/eo3jbco1s
`_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED2Ev` is the function where I added an extra `dead_on_return` to the this pointer.

> I've updated the patch at @dwblaikie's suggestion to refactor out `__reset_internal_buffer()` into two functions so that we can keep all the code reused, consolidate where everything gets reset into a similar number of lines, while preventing zeroing stores inside the destructor. I think that should help with the maintenance concerns at least a little bit.

Yeah, I'm a bit more happy with the refactoring. Still not a huge fan though.

> Rolling out `dead_on_return` on `this` on destructors will be a major behavior change. The work involved in rolling out such a change would be measured in quarters. It's totally impractical to simply make this the compiler's problem. I feel **strongly** that the practical thing to do here is not emit the store so we don't have to carry the IR to optimize it away later.

I can certainly see that rolling such a change out might take some time. However, given that it at least seems to work, do you disagree that adding `dead_on_return` to destructors is a good idea in general, and once we've done that we can basically revert anything in this patch? IIUC the actual implementation should be fairly trivial. The actual problem is that people probably sometimes rely on the compiler not optimizing dead stores in destructors. I'd be quite happy to take this patch if this is a temporary solution until we've managed to land a patch for `dead_on_return`.

> We have this problem in reverse for full constructor initialization. You end up with a bunch of zero initializing stores, and then a complex hierarchy of constructor calls that may or may not initialize the memory. The LLVM `initializes` attribute makes this slightly better, but the compiler has to give up in the general case because of things like downcasts.

I'm not sure I follow. Are you saying we're unnecessarily initializing stuff multiple times in the string constructors, or was that more of a general comment?


https://github.com/llvm/llvm-project/pull/164718