Page MenuHomePhabricator

[CSP, Cloning] Update DuplicateInstructionsInSplitBetween to use DomTreeUpdater.
ClosedPublic

Authored by fhahn on Nov 9 2018, 6:53 AM.

Details

Summary

This patch updates DuplicateInstructionsInSplitBetween to update a DTU
instead of applying updates to the DT directly.

Given that there only are 2 users, I also updated them in this patch to
avoid churn.

I slightly moved the code in CallSiteSplitting around to reduce the
places where we have to pass in DTU. If necessary, I could split those
changes in a separate patch.

This fixes missing DT updates when dealing with musttail calls in
CallSiteSplitting, by using DTU->deleteBB.

Diff Detail

Event Timeline

fhahn created this revision.Nov 9 2018, 6:53 AM
kuhar added inline comments.Nov 9 2018, 7:15 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
477–478

You can have just DTU and ask it for a DT: DTU.getDomTree(). This was it's easier to make sure you are not making any queries to an outdated DomTree.

fhahn updated this revision to Diff 173336.Nov 9 2018, 8:07 AM
fhahn marked an inline comment as done.

updated to use DTU.getDomTree(), thanks!

junbuml added inline comments.Nov 9 2018, 9:45 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
443

Now this function collects PredsWithCondsTy without performing splitting. Can you change this function name accordingly.

482–486

What about doing this in a separate function ?

NutshellySima added inline comments.Nov 9 2018, 10:37 AM
include/llvm/Transforms/Utils/Cloning.h
270

I think it's better to use a pointer to DomTreeUpdater here since DTU is not a must to use this function.

lib/Transforms/Scalar/CallSiteSplitting.cpp
457

I recommend adding assert(DTU.hasDomTree() && ...) here.

515

You need to be careful that BB isn't erased immediately because you use UpdateStrategy::Lazy.

lib/Transforms/Utils/CloneFunction.cpp
818

I have concerns on correctness here. It seems that llvm::SplitCriticalEdge used inside SplitEdge sometimes does not delete all edges between PredBB and BB. (More info)
And maybe it's better to do insertions then do deletions like this. It seems like a performance related consideration? @kuhar

unittests/Transforms/Utils/CloningTest.cpp
230

The unittest is not effective. Neither DT nor PDT is passed into DomTreeUpdater. And UpdateStrategy::Eager also needs testing.

Both DomTreeUpdater DTU(NotNullDT, NotNullPDT, Lazy) and DomTreeUpdater DTU(NotNullDT, NotNullPDT, Eager) need to be tested.

fhahn added a comment.Nov 9 2018, 10:48 AM

Thanks, I'll address the comments on Monday.

include/llvm/Transforms/Utils/Cloning.h
270

I can change it, I just figured given the few users it might be better to require DTU.

lib/Transforms/Scalar/CallSiteSplitting.cpp
515

Right, I will update the comment. Having the blocks deleted at the end of this function (or when getDomTree() is called) should be fine.

lib/Transforms/Utils/CloneFunction.cpp
818

I basically took the updates from JumpThreading. I'll take another look on Monday.

NutshellySima added inline comments.Nov 9 2018, 11:01 AM
lib/Transforms/Utils/CloneFunction.cpp
818

You can also use DTU.applyUpdates(..., ForceRemoveDuplicate=true) here to automatically remove this invalid update under Eager UpdateStrategy. But it is a side-effect of deduplication which seems like a hack and I do not recommend using it :\. (JT uses Lazy UpdateStrategy which automatically does updates deduplication).

fhahn updated this revision to Diff 173845.Nov 13 2018, 7:04 AM
fhahn marked an inline comment as done.

Address comments: renamed CSP functions, add assertion ensuring there is a single edge between PredBB and BB. I hope this addresses all comments

lib/Transforms/Scalar/CallSiteSplitting.cpp
443

I've also added short comments.

457

I've reverted the patch using the DT here, because it depends on the current patch. So this code is gone in the latest revision.

482–486

You mean calling the 2 functions above in a separate function? Given that tryToSplitCallSite itself is quite small, I think it is clearer to have to calls here directly.

lib/Transforms/Utils/CloneFunction.cpp
818

It looks like DuplicateInstructionsInSplitBetween should only be called if there is a single edge between PredBB and BB, otherwise it is not really clear what the return value should be. I have added an assertion to make sure there is only one edge between PredBB and BB (and it holds for a bootstrap build & the test suite). IIUC, this should be enough to address the issue mentioned earlier?

unittests/Transforms/Utils/CloningTest.cpp
230

I've added a comment to make clear that the DTU here is just a dummy, as the current tests do not test DT preservation. I suppose the existing JT & CSP test should cover DT preservation, but I would be happy to extend the tests in a separate patch if you think that is valuable.

NutshellySima accepted this revision.Nov 13 2018, 8:31 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Nov 13 2018, 8:31 AM
kuhar added inline comments.Nov 13 2018, 8:33 AM
lib/Transforms/Scalar/CallSiteSplitting.cpp
444

nit: missing dot at the end of the comment

unittests/Transforms/Utils/CloningTest.cpp
228

nit: missing dot at the end of the comment

277

nit: missing dot at the end of the comment

330

nit: missing dot at the end of the comment

fhahn added a comment.Nov 13 2018, 8:54 AM

Thanks, I'll update the comments before I commit this.

fhahn closed this revision.Nov 13 2018, 9:58 AM
fhahn marked 4 inline comments as done.

submitted as rL346769

Thanks for having a look!