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

Tue Jun 14 13:55:48 PDT 2022

aeubanks added a comment.

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 upstream tests move away from testing typed pointers, I'm worried that pinning to typed pointers will cause regressions that we don't want to spend time investigating. I'm not sure how long a redesign/rewrite would take so I'd like to get Chrome off of typed pointers soonish.

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

But ignoring the correctness concerns this patch was originally created to fix and looking at this patch from a "do more optimizations" standpoint this is a win because WPD will see more assume(typetests).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089