This is an archive of the discontinued LLVM Phabricator instance.

[CallSiteSplitting] Refactor creating callsites.
ClosedPublic

Authored by fhahn on Dec 1 2017, 7:33 AM.

Details

Summary

This change makes the call site creation more general if any of the
arguments is predicated on a condition in the call site's predecessors.

If we find a callsite, that potentially can be split, we collect the set
of conditions for the call site's predecessors (currently only 2
predecessors are allowed). To do that, we traverse each predecessor's
predecessors as long as it only has single predecessors and record the
condition, if it is relevant to the call site. For each condition, we
also check if the condition is taken or not. In case it is not taken,
we record the inverse predicate.

We use the recorded conditions to create the new call sites and split
the basic block.

This has 2 benefits: (1) it is slightly easier to see what is going on
(IMO) and (2) we can easily extend it to handle more complex control
flow.

Diff Detail

Event Timeline

fhahn created this revision.Dec 1 2017, 7:33 AM
junbuml edited edge metadata.Dec 1 2017, 12:14 PM

Took a quick look at this change. Look like you need to run clang-format, other than that I didn't see any issue with it. Let me take another look next week.
I believe it make this pass even better shape.

fhahn updated this revision to Diff 125233.Dec 1 2017, 3:20 PM

Thanks for having a look! I ran clang-format now.

LGTM. However, I believe we need more test cases including eq_eq_eq_taken in order to prevent any future change from breaking this pass. It will be also good to add a test case where the same argument is used multiple ICMPs.

test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll
530

In order to prevent any future change from breaking this pass, I believe we need more test cases including eq_eq_eq_taken. It will be also good to add a test case where the same argument is used multiple ICMPs.

fhahn added inline comments.Dec 6 2017, 2:53 PM
test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll
530

Agreed, I'll add such a test cases tomorrow.

junbuml added inline comments.Dec 7 2017, 8:42 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
143

We should not revisit the same predecessor. Otherwise, it might get into infinite loop if header is unreachable from the entry and we have a backedge from TBB to header :

define i32 @test(i32* %a, i32 %v, i32 %p) {
Entry:
  br label %End
Header:
  %tobool2 = icmp eq i32 %p, 10
  br i1 %tobool2, label %Tail, label %TBB
TBB:
  %cmp = icmp eq i32 %v, 1
  br i1 %cmp, label %Tail, label %Header
Tail:
  %r = call i32 @callee(i32* %a, i32 %v, i32 %p)
  ret i32 %r
End:
  ret i32 %v
}
fhahn updated this revision to Diff 125987.Dec 7 2017, 10:01 AM

Added more test cases and fixed infinite loop/

fhahn added inline comments.Dec 7 2017, 10:05 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
143

Indeed, should be fixed now. I've added this example to the tests.

test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll
530

I've added more test cases as suggested. I've also added tests where there are multiple constraints along a path (*constrain_same*). If there are conflicting conditions, e.g. x == 111 and x == 222, the first one seen will be used. I think that should be fine, as in that case the block should be unreachable. I've also mentioned that in the comment for recordConditions

junbuml accepted this revision.Dec 12 2017, 2:35 PM
junbuml added inline comments.
test/Transforms/CallSiteSplitting/callsite-split-or-phi.ll
522

unrechable -> unreachable

It might be good to add CHECK as this is also transformed with some comment about no infinite loop even in this unreachable case.

This revision is now accepted and ready to land.Dec 12 2017, 2:35 PM
fhahn closed this revision.Dec 12 2017, 7:05 PM
fhahn marked an inline comment as done.Dec 12 2017, 7:07 PM

Thank you very much, I've committed the patch with the fix to the test case.