This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port Dead Loop Deletion Pass to the new PM
ClosedPublic

Authored by junbuml on Jun 17 2016, 2:43 PM.

Diff Detail

Event Timeline

junbuml updated this revision to Diff 61134.Jun 17 2016, 2:43 PM
junbuml retitled this revision from to [PM] Port Dead Loop Deletion Pass to the new PM.
junbuml updated this object.
junbuml edited subscribers, added: llvm-commits; removed: mzolotukhin.

Hi Jun Bum,

First, thank you for taking the time to port this!

My understanding is that there is currently an issue with the new PM which causes it to not behave as expected in response to the loopInfo.markAsRemoved(L) call.
The core issue is that it is using this generic code for running a sequence of loop passes:
https://github.com/llvm-project/llvm/blob/master/include/llvm/Analysis/LoopPassManager.h#L30
https://github.com/llvm-project/llvm/blob/master/include/llvm/IR/PassManager.h#L244
Even though a loop may be marked as deleted, it will continue to run passes on it.

In the old PM, it will actually check if the loop was deleted and avoid running further passes on it:
https://github.com/llvm-project/llvm/blob/master/lib/Analysis/LoopPass.cpp#L204
https://github.com/llvm-project/llvm/blob/master/lib/Analysis/LoopPass.cpp#L238

So my suspicion is that if you run loop passes after loop-deletion deletes a loop then the pass manager will start running passes on loops that have been deleted. What happens in that case? Will we hit assertions?

(
also, the function to loop pass adaptor does not have any special handling for loop deletion; I'm not sure what that affects.
https://github.com/llvm-project/llvm/blob/master/include/llvm/Analysis/LoopPassManager.h#L84
)

I've added Davide as a reviewer. He has been looking more closely at the loop passes and will hopefully be able to provide some more information.

davide edited edge metadata.Jun 24 2016, 3:38 PM

Hi Jun Bum,

First, thank you for taking the time to port this!

My understanding is that there is currently an issue with the new PM which causes it to not behave as expected in response to the loopInfo.markAsRemoved(L) call.
The core issue is that it is using this generic code for running a sequence of loop passes:
https://github.com/llvm-project/llvm/blob/master/include/llvm/Analysis/LoopPassManager.h#L30
https://github.com/llvm-project/llvm/blob/master/include/llvm/IR/PassManager.h#L244
Even though a loop may be marked as deleted, it will continue to run passes on it.

In the old PM, it will actually check if the loop was deleted and avoid running further passes on it:
https://github.com/llvm-project/llvm/blob/master/lib/Analysis/LoopPass.cpp#L204
https://github.com/llvm-project/llvm/blob/master/lib/Analysis/LoopPass.cpp#L238

So my suspicion is that if you run loop passes after loop-deletion deletes a loop then the pass manager will start running passes on loops that have been deleted. What happens in that case? Will we hit assertions?

(
also, the function to loop pass adaptor does not have any special handling for loop deletion; I'm not sure what that affects.
https://github.com/llvm-project/llvm/blob/master/include/llvm/Analysis/LoopPassManager.h#L84
)

I've added Davide as a reviewer. He has been looking more closely at the loop passes and will hopefully be able to provide some more information.

Sean is absolutely right, this pass is blocked on some missing functionality in the new LoopPM. We're still discussing on what are the correct semantics/interfaces for the CGSCCPassManager, and after that the plan is to add the missing functionality to LoopPM. Please hold on until this is unblocked. Sorry.

davide requested changes to this revision.Jun 25 2016, 2:17 PM
davide edited edge metadata.
This revision now requires changes to proceed.Jun 25 2016, 2:17 PM

Thanks Seen and Davide for the comment. I will hold it till the issue is resolved.

Hi Jun,

Feel free to rebase and commit this. I was testing it yesterday (my rebased patch is here: http://reviews.llvm.org/F2159016) and I think it is fine to go in.

If anything, having this committed will make the problems in the loop pass manager clearer / more obvious to fix.

  • Sean Silva
silvas accepted this revision.Jul 12 2016, 3:03 PM
silvas edited edge metadata.
junbuml updated this revision to Diff 64012.Jul 14 2016, 11:12 AM
junbuml edited edge metadata.

Thanks Sean. I will commit this soon.

This revision was automatically updated to reflect the committed changes.