[llvm-dev] RFC: cleanup in Transforms/Utils

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] RFC: cleanup in Transforms/Utils

Tim Northover via llvm-dev
Hi,

I'm looking to see what's the best way to solve the fact that these two utils mostly do the same thing:
1. lib/Transforms/Utils/Local.cpp : MergeBasicBlockIntoOnlyPred
2. lib/Transforms/Utils/BasicBlockUtils.cpp : MergeBlockIntoPredecessor
(+cc some of the folks who touched at least one of these either originally or recently)

Brief overview:
1. MergeBasicBlockIntoOnlyPred2. MergeBlockIntoPredecessor
Update DTUpdate DT
Update either DT or DDTUpdates LI and MemoryDependenceResults
Move all instructions from Pred to BB, delete PredMove all instruction from BB to Pred, delete BB
assert BB has single predecessorreturn if BB doesn't have a single predecessor


Can I get some background on why there are two methods with such similar functionality?

Are folks ok with merging them into one?

If against merging, can we at least have both move instructions into the same direction (perhaps into Pred according to both function names)?

Please let me know your thoughts/preferences. I'd like to send up a cleanup patch soon for this.

Thanks,
Alina

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: cleanup in Transforms/Utils

Tim Northover via llvm-dev


On Wed, Jun 13, 2018 at 2:04 PM, Alina Sbirlea <[hidden email]> wrote:
Hi,

I'm looking to see what's the best way to solve the fact that these two utils mostly do the same thing:
1. lib/Transforms/Utils/Local.cpp : MergeBasicBlockIntoOnlyPred
2. lib/Transforms/Utils/BasicBlockUtils.cpp : MergeBlockIntoPredecessor
(+cc some of the folks who touched at least one of these either originally or recently)

Brief overview:
1. MergeBasicBlockIntoOnlyPred2. MergeBlockIntoPredecessor
Update DTUpdate DT
Update either DT or DDTUpdates LI and MemoryDependenceResults
Move all instructions from Pred to BB, delete PredMove all instruction from BB to Pred, delete BB
assert BB has single predecessorreturn if BB doesn't have a single predecessor


Can I get some background on why there are two methods with such similar functionality?


I can't comment on whether there are two methods, but I wanted to merge them myself at some point (although other priorities showed up).
I think we should keep the more general version and it should preserve the analyses.

Thanks,

--
Davide 

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: cleanup in Transforms/Utils

Tim Northover via llvm-dev
Let me add one more specific question.
One method returns (does nothing) if BB.hasAddressTaken, while the other replaces the block address with constant 1? I have zero background of the usecase for the latter, so comments welcome.

I do agree with having a general version, they each have some functionality the other does not, so the merge would be a superset of the two updating all analyses when available.


On Wed, Jun 13, 2018 at 2:09 PM, Davide Italiano <[hidden email]> wrote:


On Wed, Jun 13, 2018 at 2:04 PM, Alina Sbirlea <[hidden email]> wrote:
Hi,

I'm looking to see what's the best way to solve the fact that these two utils mostly do the same thing:
1. lib/Transforms/Utils/Local.cpp : MergeBasicBlockIntoOnlyPred
2. lib/Transforms/Utils/BasicBlockUtils.cpp : MergeBlockIntoPredecessor
(+cc some of the folks who touched at least one of these either originally or recently)

Brief overview:
1. MergeBasicBlockIntoOnlyPred2. MergeBlockIntoPredecessor
Update DTUpdate DT
Update either DT or DDTUpdates LI and MemoryDependenceResults
Move all instructions from Pred to BB, delete PredMove all instruction from BB to Pred, delete BB
assert BB has single predecessorreturn if BB doesn't have a single predecessor


Can I get some background on why there are two methods with such similar functionality?


I can't comment on whether there are two methods, but I wanted to merge them myself at some point (although other priorities showed up).
I think we should keep the more general version and it should preserve the analyses.

Thanks,

--
Davide 


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] RFC: cleanup in Transforms/Utils

Tim Northover via llvm-dev

On Wed, Jun 13, 2018 at 2:19 PM, Alina Sbirlea <[hidden email]> wrote:
Let me add one more specific question.
One method returns (does nothing) if BB.hasAddressTaken, while the other replaces the block address with constant 1? I have zero background of the usecase for the latter, so comments welcome.

I do agree with having a general version, they each have some functionality the other does not, so the merge would be a superset of the two updating all analyses when available.


On Wed, Jun 13, 2018 at 2:09 PM, Davide Italiano <[hidden email]> wrote:


On Wed, Jun 13, 2018 at 2:04 PM, Alina Sbirlea <[hidden email]> wrote:
Hi,

I'm looking to see what's the best way to solve the fact that these two utils mostly do the same thing:
1. lib/Transforms/Utils/Local.cpp : MergeBasicBlockIntoOnlyPred
2. lib/Transforms/Utils/BasicBlockUtils.cpp : MergeBlockIntoPredecessor
(+cc some of the folks who touched at least one of these either originally or recently)

Brief overview:
1. MergeBasicBlockIntoOnlyPred2. MergeBlockIntoPredecessor
Update DTUpdate DT
Update either DT or DDTUpdates LI and MemoryDependenceResults
Move all instructions from Pred to BB, delete PredMove all instruction from BB to Pred, delete BB
assert BB has single predecessorreturn if BB doesn't have a single predecessor


Can I get some background on why there are two methods with such similar functionality?


I can't comment on whether there are two methods, but I wanted to merge them myself at some point (although other priorities showed up).
I think we should keep the more general version and it should preserve the analyses.

Thanks,

--
Davide 



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev