[Devirtualization] insert placement new barrier with -O0

[Prazek]

Summary

To not break LTO with different optimizations levels, we should insert
the barrier regardles of optimization level.

[rjmccall]

Won't you have the exact same problem when LTO'ing with code that wasn't compiled with strict v-table pointers?

[mehdi_amini]

To not break LTO with different optimizations levels, we should insert the barrier regardles of optimization level.

What do you mean by "break"? Is it a correctness issue?

[Prazek]

In D32401#734870, @rjmccall wrote:

Won't you have the exact same problem when LTO'ing with code that wasn't compiled with strict v-table pointers?

No, because we fire error when combining module compiled with and without StrictVtablePointers

In D32401#734879, @mehdi_amini wrote:

To not break LTO with different optimizations levels, we should insert the barrier regardles of optimization level.

What do you mean by "break"? Is it a correctness issue?

Yes, if we would combine module with barriers and without.
You can check out this commit: https://reviews.llvm.org/D12580

[rjmccall]

I continue to be really uncomfortable with the entire design of this optimization, which appears to miscompile code by default, but as long as nobody's suggesting that we actually turn it on by default, I guess it can be your little research-compiler playground. It might be better to downgrade it to a -cc1 option, though.

This specific change is fine by me.

[Prazek]

In D32401#734921, @rjmccall wrote:

I continue to be really uncomfortable with the entire design of this optimization, which appears to miscompile code by default, but as long as nobody's suggesting that we actually turn it on by default, I guess it can be your little research-compiler playground. It might be better to downgrade it to a -cc1 option, though.

This specific change is fine by me.

Can you tell me a little more about what part of the design you dislike? Is it about missing optimizations by introducing the barriers, cost of inserting the barriers or the fact that we have to be cautious to not break anything?

[rjmccall]

In D32401#735127, @Prazek wrote:

In D32401#734921, @rjmccall wrote:

I continue to be really uncomfortable with the entire design of this optimization, which appears to miscompile code by default, but as long as nobody's suggesting that we actually turn it on by default, I guess it can be your little research-compiler playground. It might be better to downgrade it to a -cc1 option, though.

This specific change is fine by me.

Can you tell me a little more about what part of the design you dislike? Is it about missing optimizations by introducing the barriers, cost of inserting the barriers or the fact that we have to be cautious to not break anything?

The latter. The optimization design seems to rely on anticipating every case that should disable the optimization, hence this patch adding special-case logic to the frontend, and the 3 other patches you've got out for review adding special-case logic to different parts of the frontend, all on top of a ton of special-case logic in yet more parts of the frontend from when you implemented the optimization in the first place. There is an additive problem here where suddenly the design of this specific optimizaton becomes an affirmative burden to basically all the code in the frontend and, presumably, the middle-end and beyond, as opposed to just defaulting to correct behavior. There is zero chance that this latest collection of changes is actually fixing all of the problems; it's just papering over the next round of testing.

I'm very sympathetic, because I know this is an important optimization, but it's not clear to me that it's actually reasonable to implement in LLVM.

John.

[mehdi_amini]

I tend to agree with @rjmccall on the principle. Howerever:

The optimization design seems to rely on anticipating every case that should disable the optimization, hence this patch adding special-case logic to the frontend, and the 3 other patch

I believe this is patch is *removing* a special case logic that was wrong.

I agree that the whole system should be "safe". But I'm not sure it is unsafe as it is designed: if we are already issuing an error when merging two modules that have different settings IIUC.

[Prazek]

In D32401#738513, @rjmccall wrote:

In D32401#735127, @Prazek wrote:

In D32401#734921, @rjmccall wrote:

I continue to be really uncomfortable with the entire design of this optimization, which appears to miscompile code by default, but as long as nobody's suggesting that we actually turn it on by default, I guess it can be your little research-compiler playground. It might be better to downgrade it to a -cc1 option, though.

This specific change is fine by me.

Can you tell me a little more about what part of the design you dislike? Is it about missing optimizations by introducing the barriers, cost of inserting the barriers or the fact that we have to be cautious to not break anything?

The latter. The optimization design seems to rely on anticipating every case that should disable the optimization, hence this patch adding special-case logic to the frontend, and the 3 other patches you've got out for review adding special-case logic to different parts of the frontend, all on top of a ton of special-case logic in yet more parts of the frontend from when you implemented the optimization in the first place. There is an additive problem here where suddenly the design of this specific optimizaton becomes an affirmative burden to basically all the code in the frontend and, presumably, the middle-end and beyond, as opposed to just defaulting to correct behavior. There is zero chance that this latest collection of changes is actually fixing all of the problems; it's just papering over the next round of testing.

I'm very sympathetic, because I know this is an important optimization, but it's not clear to me that it's actually reasonable to implement in LLVM.

John.

We will probably not gonna make it in the first time, but the issues I am solving are known almost from the beginning - we knew that there is an issue with comparing pointers, and the idea of putting barriers for comparision was showed on last summer. Adding barriers for pointer casts is the consequence of that - either we will add
barriers for comparisions of all pointers , or only to the ones having any vptrs + add barriers when we loose the dynamic information.
The issues with linking is also known, I just solve some small details that we missed :) I hope I will be able to show some statistics soon on how devirtualization is doing to show that it is worth the cost :)

[Prazek]

Is everyone ok with sending this patch?

[rjmccall]

In D32401#759223, @Prazek wrote:

Is everyone ok with sending this patch?

As long as this continues to be an opt-in optimization, go ahead and do your research project, I suppose.

[mehdi_amini]

LGTM.

lib/CodeGen/CGExprCXX.cpp

allocation = CGM.getCXXABI().InitializeArrayCookie(*this, allocation,
                                                        E, allocType);
   }
 
   llvm::Type *elementTy = ConvertTypeForMem(allocType);
   Address result = Builder.CreateElementBitCast(allocation, elementTy);
 
   // Passing pointer through invariant.group.barrier to avoid propagation of
   // vptrs information which may be included in previous type.
+  // To not break LTO with different optimizations levels, we do it regardless
+  // of optimization level.
   if (CGM.getCodeGenOpts().StrictVTablePointers &&
-      CGM.getCodeGenOpts().OptimizationLevel > 0 &&
       allocator->isReservedGlobalPlacementOperator())
     result = Address(Builder.CreateInvariantGroupBarrier(result.getPointer()),
                      result.getAlignment());
 
   EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
                      allocSizeWithoutCookie);
   if (E->isArray()) {
     // NewPtr is a pointer to the base element type.  If we're