This is an archive of the discontinued LLVM Phabricator instance.

[LoopSimplify][NFC] Replace depth-first order process as depth_first accessor.
Needs RevisionPublic

Authored by StephenFan on Aug 12 2022, 9:43 PM.

Details

Diff Detail

Event Timeline

StephenFan created this revision.Aug 12 2022, 9:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 9:43 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
StephenFan requested review of this revision.Aug 12 2022, 9:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 9:43 PM
reames requested changes to this revision.Aug 15 2022, 8:12 AM
reames added inline comments.
llvm/lib/Transforms/Utils/LoopSimplify.cpp
731

This looks wrong. The original loop was pushing elements in inverse depth first order (i.e. from the root down) so that it could walk backwards through that order to get dfs. You seem to be visiting each item in dfs, and adding them to worklist, and then still visiting them in inverse order.

This revision now requires changes to proceed.Aug 15 2022, 8:12 AM
StephenFan added inline comments.Aug 15 2022, 9:10 AM
llvm/lib/Transforms/Utils/LoopSimplify.cpp
731

Thanks! Indeed, I was wrong. I found that the inverse_depth_first iterator accessor might be suitable.

StephenFan added inline comments.Aug 16 2022, 8:33 AM
llvm/lib/Transforms/Utils/LoopSimplify.cpp
731

After digging into the implementation detail of depth_first, I feel like you may have misunderstood what depth_first would do. I found that depth_first would return nodes from the root down. So this patch looks correct.

fhahn added a subscriber: fhahn.Aug 16 2022, 8:37 AM
fhahn added inline comments.
llvm/lib/Transforms/Utils/LoopSimplify.cpp
731

You should be able to verify that the contents of the worklist stays the same by computing it both ways and then comparing them.

StephenFan added inline comments.Aug 17 2022, 1:11 AM
llvm/lib/Transforms/Utils/LoopSimplify.cpp
731

I compared them and found the order in worklist is indeed different. The original process is actually a breadth-first iteration, and then visit them from back to front in the following while loop. Interestingly, no test is a failure despite the different iteration order.

I drew a picture to show these two different orders.