[PATCH] D44627: [CallSiteSplitting] Only record conditions up to the IDom(call site).

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

Thanks for doing this. Now, you intended it to be a NFC, right?  If then, it will be good to keep the original test cases as it is. Adding more tests is definitely good.



================
Comment at: lib/Transforms/Scalar/CallSiteSplitting.cpp:468-479
+  // Record common conditions starting from StopAt. Those conditions hold for
+  // all paths to CS. Adding them gives the inliner a better chance at inlining
+  // CS.
+  ConditionsTy CommonConditions;
+  if (StopAt)
+    recordConditions(CS, StopAt, CommonConditions, nullptr);
+  if (!CommonConditions.empty())
----------------
I think we should move this to line 461. Otherwise, we wont be able to handle the case where a known value is not detected in immediate predecessors, but there is a known value after IDom. 

Looks like test_cond_no_effect() shows the case. 


================
Comment at: test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll:133
   %tobool1 = icmp ne i32* %a, null
-  br i1 %tobool1, label %Header2, label %End
 
----------------
We should keep this test to check that nonnull is passed to both call-sites.


================
Comment at: test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll:133
   %tobool1 = icmp ne i32* %a, null
-  br i1 %tobool1, label %Header2, label %End
+  br i1 %tobool1, label %Header2, label %TBB
 
----------------
junbuml wrote:
> We should keep this test to check that nonnull is passed to both call-sites.
We can add this test, but I don't think there is any change in this test with/without this patch. 


================
Comment at: test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll:185
 ;CHECK: ret i32 %[[MERGED]]
 define i32 @test_ne_ne_ne_constrain_same_pointer_arg(i32* %a, i32 %v, i32 %p, i32* %a2, i32* %a3) {
 Header:
----------------
We should keep this test to check that nonnull is passed to both call-sites.


https://reviews.llvm.org/D44627