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

Fri Jun 10 06:19:53 PDT 2022

tejohnson added a comment.

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

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

Ok, I wasn't sure if you meant that or another broader concern about having no globals associated with the type. I think what you are saying is that the handling in [1] is a result of the visibility change I made, and that's why we need to still try to remove those assume(type.test) sequences, and thus hit the issue with this IR pattern - is that correct?

>> 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 assume(type.test) 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.

I'm not convinced that we cannot hit either of these situations, particularly [2] in cases with hidden visibility, and I'd like to confirm whether or not that's the case before changing the IR to add new intrinsics.
How about this, I propose:

1. Approve this patch, since it makes things strictly better and unblocks the miscompile. While I suppose we could in theory end up with other types of instructions between the type test and the assume use, I'm not sure in practice what that could be beyond a phi from assume merging?
2. I'll investigate more to see if I can prove or disprove that we have to remove assume(type.test) for one of the existing reasons ([1] or [2]) for cases with hidden visibility, and then we'll go from there.

FTR, I don't think this issue has anything to do with the splitting of LTT. Recall that we were also trying to remove *all* the assume(type.test) sequences before that change (D73242 <https://reviews.llvm.org/D73242>), the question is just whether the couple instances where I found we still needed to remove them ([1] and [2]) are due to the earlier visibility change (D71913 <https://reviews.llvm.org/D71913>) or not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089