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

[Tim Northover via llvm-commits]

t.p.northover added inline comments.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.h:172-175
@@ -149,1 +171,6 @@
+  /// \returns The number of bytes added to the block.
+  unsigned insertUnconditionalBranch(MachineBasicBlock &MBB,
+                                     MachineBasicBlock &DestBB,
+                                     const DebugLoc &DL,
+                                     int64_t BrOffset = 0) const;
 
----------------
arsenm wrote:
> t.p.northover wrote:
> > Could these APIs be expressed in terms of InsertBranch? At the moment it seems like we're crafting a parallel set of special-purpose branch-mangling functions but instead we could add something like
> > 
> >     SmallVectorImpl<MachineOperand> getUnconditionalCond();
> >     SmallVectorImpl<MachineOperand> getInvertedCond(SmallVectorImpl<MachineOperand> &Cond);
> > 
> > (feel free to bikeshed the interface, I'm not attached) and then these 3 become fairly simple wrappers around the existing functionality.
> I don't think it's quite the same. I thought about trying to use the existing family of analyzeBranch functions, but I think they have a different purpose. A better name would be something like analyzeBlockBranches. I hadn't thougt about trying to produce the cond vector from some other means.
> 
> The important features this one has are supporting inserting an additional branch block, passing the branch offset and returning the change in code size. I also may eventually want to introduce the ability to add even more blocks during he unconditional expansion, so at that point I don't think it has much in common with the existing InsertBranch, which just inserts a simple assumed legal branch. I suppose I could change the caller of this for conditional branches to use that instead of using 0 to assume a legal offset
> I don't think it's quite the same. I thought about trying to use the existing family of analyzeBranch functions, but I think they have a different purpose. A better name would be something like analyzeBlockBranches.

Sure, but these new functions are actually being used in a very similar way already.

  * The setDestBlock/insertInvertedBranch pair is essentially an insertBranch with inverted condition and the specified blocks.
  * insertInverted/insertUnconditional ditto.
  * The lone insertUnconditional (patching a fallthrough) is just an InsertBranch with only one BB.

> The important features this one has are supporting inserting an additional branch block

Not sure I follow this one. Are you planning to make the TargetInstrInfo do part of the relaxation (creating BBs if the unconditional branch is out of range)? That sounds like the wrong place if so.

> passing the branch offset 

I take it this is going to be used for AMDGPU? It seems unused here, and I can't quite see the implications.

> and returning the change in code size.

That is unfortunate. It still seems like it'd be better to extend the InsertBranch interface rather than duplicate it though. Simplest would be returning an `std::pair<unsigned, unsigned>`, or perhaps adding a helper to convert from NumInstructions to a size.


https://reviews.llvm.org/D23269