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

[Nikolas Klauser via libcxx-commits]

philnik777 wrote:

> > Could you demonstrate the phase ordering problem please? Looking at the pipeline it seems to me like the dse is done before inlining, and the store is in fact eliminated in the example I gave, even though the destructor (which I annotated manually) did get inlined.
> 
> The example of just calling the destructor inside a function like in the example:
> 
> ```c++
> auto test(std::string& str) {
>   str.~basic_string();
> }
> ```
> 
> is not complex enough to demonstrate the problem. The store is still trivially dead at the end of the function and can be eliminated. The pass pipeline (taken from `opt <ll file> -passes="default<O3>" -print-pipeline-passes -disable-output`):
> 
> ```
> <snip>,cgscc(devirt<4>(inline,function-attrs<skip-non-recursive-function-attrs>,argpromotion,openmp-opt-cgscc,function<eager-inv;no-rerun>(dse,<snip>
> ```
> 
> Shows that DSE runs (quite a bit but not shown in the example) after we run inlining.

That is true, but I think you're misinterpreting. My understanding (which could be wrong as well) is that code gets inlined _into_ the current function. Then, on that function, the following passes are run. After all the following passes are run, another function may inline the just optimized function. In this case that would mean that we first inline code into `~basic_string()`, do a bunch of passes, and then run DSE. After that is done `~basic_string()` is inlined into other functions. This seems to be what is going on in my example as well from looking at the pipeline on Godbolt. AFAICT DSE is first run and then `~basic_string()` is inlined into `test`, which makes it irrelevant how complex `test` actually is.

> Once this destructor gets inlined into a function of any reasonable size, Alias Analysis becomes very intractable to actually prove that the store does not alias anything, preventing DSE.
> 
> `dead_on_return` does not help here because it does not propagate anything through inlining. After inlining a function that marks a parameter as `dead_on_return`, we completely lose information about the lifetime of that parameter. That's probably possible to fix, but definitely high effort as it would require some reasonably big IR changes.
> 
> I can come up with an example walking through the pass pipeline demonstrating this, but it might be nontrivial which is why I haven't done so yet.

If my understanding above is incorrect, that I agree that all of this follows.

FWIW I'd be happy to have a short call to align our understanding of the problem here.

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