[PATCH] D23269: AArch64: Move remaining target specific BranchRelaxation bits to TII

[Matt Arsenault via llvm-commits]

arsenm added a comment.

In https://reviews.llvm.org/D23269#517159, @t.p.northover wrote:

> > I think it might be just a question of naming.
>
>
> For me it's the code duplication, though I think we might be in agreement now and just looking at it from different angles. AArch64BranchRelaxation contains quite a bit of code basically copy/pasted from AArch64InstrInfo.cpp just because the Analyze/Insert/Remove branch interface wasn't quite right. This looks even sillier when both instances are in InstrInfo with subtly different semantics.
>
> Adding a convenience `insertUnconditionalBranch` function (and even `insertInvertedConditionBranch`) seems reasonable, but one way or the other delegation with InsertBranch should be involved.
>
> Anyway, a very preliminary and largely untested outline of what I have in mind is at https://github.com/TNorthover/llvm/tree/branch-relax (making use of a possibly buggy version of https://reviews.llvm.org/D23379 so we can analyze everything). The diff is +57/-174 at the moment. It seems like we'd just have to make InsertBranch return the bytes as well (if not for IfConversion we could do a straight swap) and accept optional `KnownOffset` args and it could do everything you need.


We could get away with it returning the number of instructions to get the size by calling getInstSizeInBytes on them (though I always thought the return the number of instructions API was weird). Your patch untangles the weird mess with NeedSplit trying to skip analyzeBranch which I was hoping to fix later, so overall I think your patch makes sense. The API changes I really need make more sense in the context of inserting an indirect branch, so I think keeping those bits as a new function separate from InsertBranch still makes sense rather than cluttering up all of the other uses


https://reviews.llvm.org/D23269