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

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:114-115
@@ -113,2 +113,4 @@
     llvm_unreachable("unexpected opcode!");
+  case AArch64::B:
+    return 64;
   case AArch64::TBNZW:
----------------
t.p.northover wrote:
> This doesn't look right to me. I see that it's coming from a "return -1" but unconditional branches don't have unlimited range. They've got 26-bit signed range.
The current code assumes they don't need to be relaxed, so I just assumed infinite range. I have another patch which adds support for expanding unconditional branches

================
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;
 
----------------
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


https://reviews.llvm.org/D23269