[llvm] [WholeProgramDevirt] Add check for AvailableExternal and give up devirt (PR #143468)

[Teresa Johnson via llvm-commits]

teresajohnson wrote:

> Hi @teresajohnson Thanks for your review.
> 
> > I don't see them checking isDeclarationForLinker or the linkage type directly.
> 
> I mean vtable's all function's body would be checked. If any of the functions is a declaration, devirtualization of tryVirtualConstProp would drop. For example of "@_ZTVSt9exception = available_externally constant { [2 x ptr] } { [2 x ptr] [ptr foo1, ptr foo2] }". If foo1 or foo2 is a declaration, devirtualization would be drop. Code in tryVirtualConstProp,
> 
> https://github.com/llvm/llvm-project/blob/968d8eaa44c500259fe8d56ad77ec1c71cad35e2/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp#L1868
> 
> 
> In code, checking function by isDeclaration() but not isDeclarationForLinker(). So I guess if function is available_externally, the devirt would not drop. But I am curious what a real case that a external function is regarded as available_externally?

Not sure but we should probably be consistent unless there is a reason not to.

> **You are right** for trySingleImplDevirt that it does not check function's body. No matter if vtable is available_externally or not, it would do devirtualization if vtable's function are the same. The example is as below: `@_ZTVSt9exception = **available_externally** constant { [2 x ptr] } { [2 x ptr] [ptr null, ptr @foo] }` `@_ZTV1A.0 = constant { [2 x ptr] } { [2 x ptr] [ptr null, ptr @foo] }` trySingleImplDevirt() would work and translate to `tail call i16 @foo(ptr null)` So for trySingleImplDevirt, it does not check available_externally. **If no check of available_externally is reasonable for trySingleImplDevirt, I think I should move the checking in DevirtModule::tryICallBranchFunnel**.
> 
> > Do you have a test case where all vtables are available externally, and how does that work?
> 
> I write a all vtables are available externally and it crashes in LowerTypeTests. The crash is because it skips dealing with available externally. And code is at
> 
> https://github.com/llvm/llvm-project/blob/9d491bc602c2d9730cb42fe25f0753471a3af389/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L2227
> 
> 
> Even I walk around it, there is still linkage type issue that not pass VerifierPass because **all available externally always combined to privateLinkage GV** at
> https://github.com/llvm/llvm-project/blob/9d491bc602c2d9730cb42fe25f0753471a3af389/llvm/lib/Transforms/IPO/LowerTypeTests.cpp#L887
> 
> 
> For example:
> `@_ZTVSt9exception = available_externally constant { [5 x ptr] } { [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNKSt9exception4whatEv] }`
> `@_ZTV1A.0 = available_externally constant [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNK1A4whatEv]`
> tends to combine to:
> `@1 = private constant { { [5 x ptr] }, [24 x i8], [5 x ptr] } { { [5 x ptr] } { [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNKSt9exception4whatEv] }, [24 x i8] zer oinitializer, [5 x ptr] [ptr null, ptr null, ptr null, ptr null, ptr @_ZNK1A4whatEv] }, align 8`
> `@_ZTVSt9exception = available_externally alias { [5 x ptr] }, ptr @1`
> `@_ZTV1A.0 = available_externally alias [5 x ptr], getelementptr inbounds ({ { [5 x ptr] }, [24 x i8], [5 x ptr] }, ptr @1, i32 0, i32 2)`
> I am not sure if they are bugs for LowerTypeTests?
> **If assuming LowerTypeTests is right, I guess any available externally for icall.branch.funnel should be stopped**.
> So do you agree that I postpone available externally in tryICallBranchFunnel, and any one of available externally vtable would stop icall.branch.funnel?

This sounds good, since it is the icall.branch.funnel that has an issue with these vtables, go ahead and put the checks there. I'm not sure if this is a limitation of LowerTypeTests, but in the absence of input from someone more familiar with this intrinsics it makes sense to be conservative in apply that transformation.

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