Page MenuHomePhabricator

[Dominators] Teach LoopUnswitch to use the incremental API
ClosedPublic

Authored by kuhar on Jul 17 2017, 6:53 PM.

Details

Summary

This patch makes LoopUnswitch use new incremental API for updating dominators.
It also updates SplitCriticalEdge, as it is called in LoopUnswitch.

There doesn't seem to be any noticeable performance difference when bootstrapping clang with this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

kuhar created this revision.Jul 17 2017, 6:53 PM
kuhar removed a reviewer: dgross.
grosser accepted this revision.Jul 17 2017, 8:31 PM

This looks good to me! Thank you.

lib/Transforms/Scalar/LoopUnswitch.cpp
1260 ↗(On Diff #107009)

It IS no longer needed, so delete it.

lib/Transforms/Utils/BreakCriticalEdges.cpp
227 ↗(On Diff #107009)

Very nice cleanup, indeed!

This revision is now accepted and ready to land.Jul 17 2017, 8:31 PM
davide edited edge metadata.Jul 17 2017, 9:16 PM

I'm a little fried at the moment, so just some drive-by comments. I wonder if we can make the memory management automatic instead of calling naked delete.

lib/Transforms/Scalar/LoopUnswitch.cpp
938 ↗(On Diff #107009)

message in the assertion?

939 ↗(On Diff #107009)

instead of using isa<> + cast<> can you use dyn_cast<> maybe ?

lib/Transforms/Utils/BreakCriticalEdges.cpp
224–227 ↗(On Diff #107009)

Can you add a more descriptive comment here?

kuhar updated this revision to Diff 107124.Jul 18 2017, 9:55 AM

Improve comments

lib/Transforms/Scalar/LoopUnswitch.cpp
939 ↗(On Diff #107009)

Wouldn't that be slower in release builds without assertions?

kuhar updated this revision to Diff 107128.Jul 18 2017, 10:01 AM

Improve LoopUnswitch assertion.

kuhar marked 3 inline comments as done.Jul 18 2017, 10:01 AM
davide accepted this revision.Jul 18 2017, 11:27 AM

The logic is fine. Thanks!

kuhar updated this revision to Diff 109181.Aug 1 2017, 12:29 PM

Use the new batch update API.

This revision was automatically updated to reflect the committed changes.