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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D126089#3571368 <https://reviews.llvm.org/D126089#3571368>, @tejohnson wrote:

> In D126089#3571208 <https://reviews.llvm.org/D126089#3571208>, @pcc wrote:
>
>> In D126089#3570116 <https://reviews.llvm.org/D126089#3570116>, @tejohnson wrote:
>>
>>> 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.
>>
>> The problem with this design is that it leaves the meaning of `llvm.type.test` undefined in the case where there are no globals that are a member of the type, because the `!vcall_visibility` is associated with the global. So there's no way to know whether the type argument was intended to refer to a type with hidden LTO visibility or one with public LTO visibility, if there are no member globals.
>>
>> Since `-lto-whole-program-visibility` is a flag that affects the operation of the type tests globally, I think a better design would be to replace all `llvm.type.test` calls with `true`, and replaces `llvm.type.checked.load` with a regular load, if the flag is not passed.
>
> We only perform WPD if there is a global, however. And then we subsequently remove these sequences if there is no global for a type id [1].
>
> Maybe I am misunderstanding the concern though, can you give an example where this will go wrong?

I think Arthur's reproducer is exactly an example of where this will go wrong. Any IR which is not in the exact pattern expected by WPD (phi is just one example but who knows what passes will do in the future) will cause WPD to leave the IR in an incorrect form that will cause problems for LTT.

> Also, is it the case that we can only get into the case [1], where the type id is not used on a global, with my changes to insert type test for non-hidden visibility?
>
> [1] https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp#L1942-L1945
>
> Another case I found where I had to keep the removal of the type test assume sequences is described in [2]. I'm not sure that has anything to do with the visibility.
>
> [2] https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp#L1947-L1965

I strongly suspect that we should be able to remove the code handling both of those cases if we fix the root cause here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089