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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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())
----------------
junbuml wrote:
> junbuml wrote:
> > 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. 
> In this case (test_cond_no_effect), we should just pass the known value without splitting.  Looks like I tried thing similar before in https://reviews.llvm.org/D41782. 
As this patch is at the moment, it is not a NFC, as it prevents splitting in case there are no suitable conditions along any path between the IDom and the call site.

>In this case (test_cond_no_effect), we should just pass the known value without splitting. Looks like I tried thing similar before in https://reviews.llvm.org/D41782.

I just thought the same thing :) With my change, it should be quite straight forward to handle that case. But I would prefer to do that in a different patch


https://reviews.llvm.org/D44627