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

Wed May 25 20:04:48 PDT 2022

tejohnson added a comment.

In D126089#3538969 <https://reviews.llvm.org/D126089#3538969>, @pcc wrote:

> In D126089#3538950 <https://reviews.llvm.org/D126089#3538950>, @aeubanks wrote:
>
>> In D126089#3538842 <https://reviews.llvm.org/D126089#3538842>, @pcc wrote:
>>
>>> 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?
>>
>> I'm not super familiar with ThinLTO summaries, could you explain a bit more?
>> The assume(phi(type test, type test)) is coming from the output of the pre link. Then post link WPD looks at all type test usages and I think LTT does the same? So not sure where the summary is relevant.
>
> The summary is meant to control what actions are done by the LTT and WPD passes later. For example here is the summary with an `assume(phi(type.test, type.test))` which simulates what you told me that the phi comes from the output of the pre link:
>
>   > cat foo.ll 
>   define i1 @f() {
>     br i1 undef, label %bb1, label %bb2
>   
>   bb1:
>     %p1 = call i1 @llvm.type.test(i8* null, metadata !"foo")
>     br label %end
>   
>   bb2:
>     %p2 = call i1 @llvm.type.test(i8* null, metadata !"bar")
>     br label %end
>   
>   end:
>     %p = phi i1 [ %p1, %bb1 ], [ %p2, %bb2 ]
>     call void @llvm.assume(i1 %p)
>     ret i1 %p
>   }
>   
>   declare i1 @llvm.type.test(i8*, metadata) nounwind readnone
>   declare void @llvm.assume(i1)
>   > ra/bin/opt -module-summary foo.ll -o foo.bc && ra/bin/llvm-dis -o - foo.bc
>   ; ModuleID = 'foo.bc'
>   source_filename = "foo.ll"
>   
>   define i1 @f() {
>     br i1 undef, label %bb1, label %bb2
>   
>   bb1:                                              ; preds = %0
>     %p1 = call i1 @llvm.type.test(i8* null, metadata !"foo")
>     br label %end
>   
>   bb2:                                              ; preds = %0
>     %p2 = call i1 @llvm.type.test(i8* null, metadata !"bar")
>     br label %end
>   
>   end:                                              ; preds = %bb2, %bb1
>     %p = phi i1 [ %p1, %bb1 ], [ %p2, %bb2 ]
>     call void @llvm.assume(i1 %p)
>     ret i1 %p
>   }
>   
>   ; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
>   declare i1 @llvm.type.test(i8*, metadata) #0
>   
>   ; Function Attrs: inaccessiblememonly nocallback nofree nosync nounwind willreturn
>   declare void @llvm.assume(i1 noundef) #1
>   
>   attributes #0 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
>   attributes #1 = { inaccessiblememonly nocallback nofree nosync nounwind willreturn }
>   
>   ^0 = module: (path: "foo.bc", hash: (0, 0, 0, 0, 0))
>   ^1 = gv: (name: "llvm.type.test") ; guid = 608142985856744218
>   ^2 = gv: (name: "llvm.assume") ; guid = 6385187066495850096
>   ^3 = gv: (name: "f", summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 8, typeIdInfo: (typeTests: (6699318081062747564, 16434608426314478903))))) ; guid = 14740650423002898831
>   ^4 = blockcount: 4
>
> This is the expected output. Note the appearance of `typeTests` in the summary, which means that a `llvm.type.test` was found that was not directly used by `llvm.assume`. This triggers the creation of a `TTRes` in the combined summary for `foo` and `bar`, as opposed to a `WPDRes` (you can generate the combined summary with `-lowertypetests-write-summary`, see e.g. `llvm/test/Transforms/LowerTypeTests/export-single.ll`). I'm guessing that in your case you are somehow ending up with `typeTestAssumes` in the summary for these type tests instead of `typeTests`, or that something else is preventing the `TTRes` from being created or causing a `WPDRes` to be created instead.

I don't think the WPDRes is relevant here? If you look at the code that is attempting to remove the type test assumes, in DevirtModule::scanTypeTests(), it is looking at the IR not the resolutions (in findDevirtualizableCallsForTypeTest at https://github.com/llvm/llvm-project/blob/09c2b7c35af8c4bad39f03e9f60df8bd07323028/llvm/lib/Analysis/TypeMetadataUtils.cpp#L74-L91), then attempting to remove the sequences from the IR (https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp#L1924-L1929). Before my change to try to keep some of these around for later use, this latter block of code was done unconditionally, and I found I still needed to remove some that would get Unsat resolutions in LTT. Unfortunately because of the phi in this case, the removal is not successful without this patch.

I'm not sure how opaque pointers suddenly started triggering this issue, however. @aeubanks, any ideas as to what change opaque pointers provokes that enables the assume merging?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126089