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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

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

> In D126089#3538760 <https://reviews.llvm.org/D126089#3538760>, @pcc wrote:
>
>> This case is supposed to be handled as a result of running the WPD pass early. See the comment near the top of PassBuilder::buildThinLTODefaultPipeline:
>>
>>   // These passes import type identifier resolutions for whole-program
>>   // devirtualization and CFI. They must run early because other passes may
>>   // disturb the specific instruction patterns that these passes look for,
>>   // creating dependencies on resolutions that may not appear in the summary.
>>   //
>>   // For example, GVN may transform the pattern assume(type.test) appearing in
>>   // two basic blocks into assume(phi(type.test, type.test)), which would
>>   // transform a dependency on a WPD resolution into a dependency on a type
>>   // identifier resolution for CFI.
>>   //
>>   // Also, WPD has access to more precise information than ICP and can
>>   // devirtualize more effectively, so it should operate on the IR first.
>>   //
>>   // The WPD and LowerTypeTest passes need to run at -O0 to lower type
>>   // metadata and intrinsics.
>>
>> Is there some way that we can now end up with GVN or some other such pass running between when we create the summary and when we run WPD that's inserting a phi?
>
> I thought the frontend emitted the type test/assume. I'm seeing them in the IR after the pre link step. `CodeGenFunction::EmitTypeMetadataCodeForVCall()` in clang emits a type test/assume.

Right, but the phi must be coming from somewhere else after summary generation if it isn't accounted for in the summary (if the phi was inserted before summary generation, the summary should end up with a dependency on a CFI type identifier resolution, instead of a WPD resolution). Can you trace where it is coming from?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089