[PATCH] D126089: [WPD] Try harder to find assumes through phis

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D126089#3569012 <https://reviews.llvm.org/D126089#3569012>, @pcc wrote:

> Thanks for the reproducer!
>
> I think the root cause of the issue here is that the IR generated by Clang is wrong. `llvm.type.test` will only consider types that are local to the LTO unit, which means that it is incorrect to emit `llvm.assume(llvm.type.test)` for types A or B, because they have public LTO visibility, which means that definitions may reside outside of the LTO unit. The LowerTypeTests pass is therefore correctly replacing the `llvm.type.test`s with false, leading to the miscompilation seen here. The issue can also be seen if the `[[clang::lto_visibility_public]]` attribute is removed.

This is by design per the changes I made awhile back to delay the decision about visibility until LTO link time: https://groups.google.com/g/llvm-dev/c/6LfIiAo9g68/m/Ng1Xvw4WAwAJ
They should have public !vcall_visibility metadata by default, and since whole program visibility isn't being asserted here, there should be no devirt, etc.

> I gather from looking at the commit history that the intent was to express information about the likelihood of particular calls being made as input to the ICP pass via these intrinsics (although it appears that this work was never completed as I can't find any code in the ICP pass that looks at these intrinsics).

Right, I have prototyped changes to allow this to guide insertion of ICP guard sequences to avoid an extra load (compare against possible vtables, not virtual function defs). I've unfortunately never had the bandwidth to clean that up and send for review, but I should carve out the time to get that upstreamed at least under an option. But note I don't think this is the cause of the failure - the code in WPD to remove these sequences (the assumes, and any type tests with no more uses) existed before I split LTT for the ICP purpose, so even without my changes to keep in some of them we would have left behind these cases incorrectly - currently we are properly detecting here that the assume(type.test) needs to be removed because LTT won't handle it as desired, but the phi is blocking the type test removal.

> I think it would be inappropriate to use `llvm.assume(llvm.type.test)` for this purpose. Instead, in the case where the type has public LTO visibility, the IR should express a strong likelihood that the `llvm.type.test` will evaluate to true, for example by using the `llvm.expect` intrinsic. Then even if an optimization like this were to occur, the IR would still boil down to `llvm.expect(false)`, which would not cause a miscompilation.

Again, this is related to the RFC linked above, and I believe when we discussed it back then you were in favor of using the same llvm.assume(llvm.type.test) sequences. Which isn't to say that it couldn't be changed, but I think the !vcall_visibility metadata handling should be enough to guide this appropriately? I think the issue here is still just that it isn't properly being cleaned up when there is a phi involved.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126089/new/

https://reviews.llvm.org/D126089