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

Thu Jun 9 14:29:35 PDT 2022

pcc added a comment.

In D126089#3571365 <https://reviews.llvm.org/D126089#3571365>, @aeubanks wrote:

>> 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.
>
> My original patch (https://reviews.llvm.org/D126089?id=431037) replaced `llvm.type.test` with `true` in all cases, but tejohnson said
>
>> Regarding the quoted comment, note it is "type test assume" sequences, not all type tests, that need to be removed here in certain circumstances. Note also that before I made the change to try to keep these around longer for use by other analyses, this code attempted to remove all "type test assume" sequences. I.e. it removed all the analyzed assumes, and any type test that was subsequently dead. Presumably that version of WPD+LTT would have also illustrated the original problem this patch tries to address (an assume fed by a phi, in turn fed by a type test that got an Unsat and resulted in llvm.assume(false)), and for a WPD+CFI build we certainly wouldn't want to replace *all* type test uses in the code at this point with true. So I am skeptical that it is correct to replace type test non-assume (non-phi) uses with true in the more limited cases being handled currently either.

Oh right, I managed to confuse myself about the semantics of not passing `-lto-whole-program-visibility`. It doesn't have the effect of giving all classes public LTO visibility, just those which were declared with public LTO visibility. The former would have some undesirable consequences for CFI.

So I think that means that WPD needs to be using a separate intrinsic for checks on classes with public LTO visibility. That would get replaced with `llvm.type.test` if  `-lto-whole-program-visibility` is passed, and `true` if it is not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089