llvm dev - Jun 2018 - RFC: cleanup in Transforms/Utils

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. MergeBasicBlockIntoOnlyPred 2. MergeBlockIntoPredecessor
Update DT Update DT
Update either DT or DDT Updates LI and MemoryDependenceResults
Move all instructions from Pred to BB, delete Pred Move all instruction
from BB to Pred, delete BB
assert BB has single predecessor return 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180613/d4321e0c/attachment.html>

On Wed, Jun 13, 2018 at 2:04 PM, Alina Sbirlea <alina.sbirlea at
gmail.com>
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. MergeBasicBlockIntoOnlyPred 2. MergeBlockIntoPredecessor
> Update DT Update DT
> Update either DT or DDT Updates LI and MemoryDependenceResults
> Move all instructions from Pred to BB, delete Pred Move all instruction
> from BB to Pred, delete BB
> assert BB has single predecessor return 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180613/4b4cd79b/attachment.html>

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 <dccitaliano at
gmail.com>
wrote:
>
>
> On Wed, Jun 13, 2018 at 2:04 PM, Alina Sbirlea <alina.sbirlea at
gmail.com>
> 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. MergeBasicBlockIntoOnlyPred 2. MergeBlockIntoPredecessor
>> Update DT Update DT
>> Update either DT or DDT Updates LI and MemoryDependenceResults
>> Move all instructions from Pred to BB, delete Pred Move all instruction
>> from BB to Pred, delete BB
>> assert BB has single predecessor return 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180613/8a639425/attachment.html>