RFC: cleanup in Transforms/Utils

Project Infrastructure LLVM Dev List Archives
#1

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

#2

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,

#3

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.

#4

Sent out: https://reviews.llvm.org/D48202