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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc requested changes to this revision.
pcc added a comment.
This revision now requires changes to proceed.

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.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089