Page MenuHomePhabricator

[WPD] Provide a way to prevent function(s) from being devirtualized
ClosedPublic

Authored by evgeny777 on Mar 4 2020, 8:43 AM.

Diff Detail

Event Timeline

evgeny777 created this revision.Mar 4 2020, 8:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 8:43 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

This won't work for the index-only WPD. Probably the best way to cover that case is to use the option in ModuleSummaryAnalysis.cpp to block functions from being added to the VTableFuncs in the summary in findFuncPointers.

llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
2

Why does skipping this one in particular cause both vf0i1 and vf1i1 to be skipped?

Also the comment here says that we won't devirtualize vf0i1 "and all other virtual call targets" except that the checking still looks for devirtualizations of vf*i32.

6

Similar question here about why skipping one blocks the devirt of the others

25

Should there be a SKIP-NOT: devirtualized above here too to make sure there are no devirt messages before this?

evgeny777 marked an inline comment as done.Mar 5 2020, 1:36 AM

This won't work for the index-only WPD.

This can be addressed at the moment, because index based WPD supports only single-impl devirtualization. In this case we have function name in the index.

Probably the best way to cover that case is to use the option in ModuleSummaryAnalysis.cpp to block functions from being added to the VTableFuncs in the summary in findFuncPointers.

I don't think this is enough. If we have call slot { foo, bar }, and want to prevent devirtualization of foo, then entire call slot can't be devirtualized. If bar is not used by any other call slot then it won't be devirtualized as well

llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
2

We have 4 vtables:

vtbl1: { vf0i1, vf1i1, vf1i32 }
vtbl2: { vf1i1, vf0i1, vf2i32 }
vtbl3: { vf0i1, vf1i1, vf3i32 }
vtbl4: { vf1i1, vf0i1, vf4i32 }

and 3 call slots:

CS1: (ByteOffset == 16) : { vf1i32, vf2i32, vf3i32, vf4i32 }
CS2: (ByteOffset == 8) : { vf1i1, vf0i1 }
CS3: (ByteOffset == 0) : { vf0i1, vf1i1 }

If we want to prevent devirtualization of vf0i1 then both CS2 and CS3 can't be devirtualzied and final devirtualized set is { vf1i32, vf2i32, vf3i32, vf4i32 }
If we want to prevent devirtualization of vf1i32 then CS1 can't be devirtualized and final devirtualized set is { vf0i1 vf1i1 }

This won't work for the index-only WPD.

This can be addressed at the moment, because index based WPD supports only single-impl devirtualization. In this case we have function name in the index.

Meaning we can just check the single impl name down in DevirtIndex::trySingleImplDevirt and block it there? Yeah, that will work.

Probably the best way to cover that case is to use the option in ModuleSummaryAnalysis.cpp to block functions from being added to the VTableFuncs in the summary in findFuncPointers.

I don't think this is enough. If we have call slot { foo, bar }, and want to prevent devirtualization of foo, then entire call slot can't be devirtualized. If bar is not used by any other call slot then it won't be devirtualized as well

Right, nevermind, we will actually do incorrect single impl devirts with what I suggested, because we will be missing some of the targets for the slot. I'm not sure I follow your second sentence here, but essentially if the targets for the slot are supposed to be {foo, bar} and if we didn't summarize foo, then we would incorrectly perform a single impl devirt to bar, when we shouldn't have devirtualized at all. Or are you saying something different?

llvm/test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll
2

Ok, nevermind, I was thinking about single impl devirt, wondering how we can devirtualize with multiple targets in the slot, but I see in this case it is because we are doing virtual constant prop.

if we didn't summarize foo, then we would incorrectly perform a single impl devirt to bar, when we shouldn't have devirtualized at all.

True

Or are you saying something different?

Actually I wanted to say that if we prevent foo from devirtualizing and have two call sites:

CS1: { foo, bar }
CS2: { bar, baz }

then CS2 will still be devirtulized, so specifying -wholeprogramdevirt-skip=foo is actually telling WPD that all call sites potentially calling foo should remain virtual.

evgeny777 updated this revision to Diff 248537.Mar 5 2020, 10:19 AM
  • Added support for index based WPD
  • Minor improvements in test case
pcc added a subscriber: pcc.Mar 5 2020, 10:49 AM

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

In D75617#1908189, @pcc wrote:

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

I assumed debugging, but will let @evgeny777 comment on his use case.

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

I think, it's not the same, because

  • it requires full recompilation, not just relink.
  • it operates on vtables not on individual functions
tejohnson added inline comments.Mar 5 2020, 10:55 AM
llvm/test/ThinLTO/X86/devirt.ll
128

This doesn't check for the absence of the _ZN1A1nEi devirt. Probably should be the following?
; SKIP-NOT: single-impl: devirtualized a call to _ZN1A1nEi

@pcc Just to clarify: I'm using this patch to identify problems with WPD (typically those are either invalid reinterpret_cast or open type hierarchy). I find it convenient to obtain a list of devirtualized functions using pass remarks, "undevirtualize" some of them relink and run. With attributes it seems to be much harder

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

It is for debugging. Not WPD itself but rather a devirtualized binary. See comment above

pcc added a comment.Mar 5 2020, 11:15 AM

@pcc Just to clarify: I'm using this patch to identify problems with WPD (typically those are either invalid reinterpret_cast or open type hierarchy). I find it convenient to obtain a list of devirtualized functions using pass remarks, "undevirtualize" some of them relink and run. With attributes it seems to be much harder

Okay. If it's just for debugging then the option needs to be made hidden.

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

It is for debugging. Not WPD itself but rather a devirtualized binary. See comment above

Sorry I skipped ahead after reading the first few comments. That sounds fine to me. Thanks for explaining.

evgeny777 updated this revision to Diff 248682.Mar 6 2020, 2:51 AM

Addressed comments

This revision is now accepted and ready to land.Mar 6 2020, 6:07 AM
This revision was automatically updated to reflect the committed changes.