This is an archive of the discontinued LLVM Phabricator instance.

[JumpThread] We want to fold (not thread) when all predecessor go to single BB's successor. .
ClosedPublic

Authored by trentxintong on Mar 11 2017, 3:02 PM.

Event Timeline

trentxintong created this revision.Mar 11 2017, 3:02 PM
majnemer added inline comments.
lib/Transforms/Scalar/JumpThreading.cpp
1297

I'd use size_t instead of unsigned here.

1298

This can be successors(BB)

1312

This can be auto *CondInst

dberlin added inline comments.
lib/Transforms/Scalar/JumpThreading.cpp
1297

If you are going to check predecessors a bunch, please use the prediteratorcache to cache the size.
This is not a cheap operation.

trentxintong added inline comments.Mar 11 2017, 5:23 PM
lib/Transforms/Scalar/JumpThreading.cpp
1297

I think thats a good suggestion. There are a few places we get the predecessors of the block currently being processed, we could use a PredIterCache for them. But I prefer to put it in as another commit.

Address comments.

Gentle Ping.

lib/Transforms/Scalar/JumpThreading.cpp
1297

dberlin is doing experiment about preditercache. I will rewrite this part if needed when that is done in another commit.

trentxintong retitled this revision from [JumpThread] In case all predecessor go to a single successor of current BB. We want to fold (not thread). to [JumpThread] We want to fold (not thread) when all predecessor go to single BB's successor. ..Mar 19 2017, 11:10 AM

Update comments and change how predecessors with known successor is computed.

Update comment

sanjoy requested changes to this revision.Apr 18 2017, 4:27 PM

Comments inline.

lib/Transforms/Scalar/JumpThreading.cpp
1289

This logic looks like it isn't tested?

In any case, I'd suggest not putting it in this first patch, but doing it as an improvement on the fold-instead-of-threading logic.

test/Transforms/JumpThreading/basic.ll
21

Can you please add a test case with multiple preds of L0?

This revision now requires changes to proceed.Apr 18 2017, 4:27 PM
trentxintong edited edge metadata.

Address sanjoy's comments

Remove PredWithKnownSuccessor. its not really needed.

sanjoy accepted this revision.Apr 18 2017, 6:17 PM

lgtm with nits

lib/Transforms/Scalar/JumpThreading.cpp
1294

s/dont/don't
s/cant/can't

1299

In this case, it may be cleaner to write the loop as:

for (BasicBlock *SuccBB : successors(BB))
  if (SuccBB != OnlyDest)
    SuccBB->removePredecessor(BB, true);  // Unreachable successor
This revision is now accepted and ready to land.Apr 18 2017, 6:17 PM

Address comments.

This revision was automatically updated to reflect the committed changes.