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

Tue Jun 14 12:24:48 PDT 2022

pcc added a comment.

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

> In D126089#3582321 <https://reviews.llvm.org/D126089#3582321>, @pcc wrote:
>
>> In D126089#3582227 <https://reviews.llvm.org/D126089#3582227>, @aeubanks wrote:
>>
>>> is this patch ok to submit at least as a short term workaround?
>>
>> Chrome already has a short-term workaround, which is to disable opaque pointers, right? I think you can use that until the proper fix is developed.
>
> As noted, I'm really not convinced that we cannot hit this issue for hidden visibility patches.
>
> However, another reason to submit this particular patch is that it is merely enabling WPD in additional cases, when there is a phi between the type test and assume, by recognizing more assumes. I would imagine we will still have cases like that even for hidden visibility classes, or with your proposed fix to use a new TBD intrinsic that is converted to an llvm.type.test in the case of --lto-whole-program-visibility, right? So in either case this seems to me to be an improvement.

For hidden LTO visibility classes the set of classes in the LTO unit is exactly the same as the set of classes which is possible at runtime, which is not the case for public LTO visibility classes. So if the type test gets detached from the assume by one of the intervening optimization passes, instead of ending up with `assume(false)` you will get `assume(useless CFI type check)`, assuming that there's at least one possible class. And that's harmless from a correctness perspective.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089