[PATCH] D103073: [LTT] Handle merged llvm.assume when dropping type tests

Thu Mar 3 10:17:22 PST 2022

tejohnson added inline comments.
Herald added a project: All.

================
Comment at: llvm/test/Transforms/LowerTypeTests/drop_type_test_phi.ll:38
+  %4 = phi i1 [ %3, %if.else ], [ %p, %if.then ]
+  call void @llvm.assume(i1 %4)
+; Still have the assume, but the type test target replaced with true.
----------------
MaskRay wrote:
> I am a bit puzzled by the comment "If this type test is only used by llvm.assume instructions": this test has exactly one assume. Where did the merge happen?
The comment you are referring to is from LowerTypeTestsModule::lower() the first time we invoke LTT, which happens before the optimization passes in the LTO backends (right after importing and WPD). There is a second invocation of LTT which simply drops the remaining type test sequences later on, and that is what is being fixed by this patch. It is after a number of standard optimization passes. It has been awhile since I fixed this but so I don't recall exactly which pass was combining the llvm.assume instructions, but it could have been GVN or InstCombine? There are no special restrictions on being able to combine llvm.assume in general.

However, you raise a good point, because the code you referred to will I believe cause the type tests to be removed earlier than desired on the first pass if it already feeds a phi (e.g. if the assume merging happened during the pre-LTO link optimization passes), so that should be loosened up a bit. IIRC the merging of assumes in my case happened after ThinLTO function importing, so didn't hit that case. Let me look at adding a patch to that code to handle the phi case. BTW this is not a correctness issue, it just means they wouldn't be around for subsequent use for optimization.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103073