This is an archive of the discontinued LLVM Phabricator instance.

[Devirtualization] insert placement new barrier with -O0
ClosedPublic

Authored by Prazek on Apr 23 2017, 10:00 AM.

Event Timeline

Prazek created this revision.Apr 23 2017, 10:00 AM
rjmccall edited edge metadata.Apr 23 2017, 1:34 PM

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

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?

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

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

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.

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?

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.

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.

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 :)

Is everyone ok with sending this patch?

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.

This revision is now accepted and ready to land.May 19 2017, 10:31 AM
This revision was automatically updated to reflect the committed changes.