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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

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?

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 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.
>
> To me that sounds like WPD is taking broken IR and attempting to "fix" it before it reaches another pass. That doesn't make for a particularly sound design because WPD would need to be able to detect all possible cases of broken IR in the presence of arbitrary optimization passes between the producer and WPD.
>
>>> 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.
>
> I'm not sure if that was my position, but perhaps I just hadn't noticed the issue with interaction with other passes.
>
>> 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.
>
> I think that just dealing with the phi is papering over the root cause of the issue. There's nothing preventing an optimization pass from inserting something else between the type test and the assume and reintroducing the same issue.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089