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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

I thought



================
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())
----------------
fhahn wrote:
> 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
> 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 thought a bit more about how to best handle propagating common facts that do not require splitting. I do not think we can do it properly here without making things to complicated. I've shared a WIP patch that is a first step towards enabling IPSCCP to propagate facts from compare instructions : D45330


https://reviews.llvm.org/D44627