[PATCH] D20162: [MSP430] PR27500 CodeGen: Rework branch-select pass

[Vadzim Dambrouski via llvm-commits]

pftbest added inline comments.

================
Comment at: lib/Target/MSP430/MSP430BranchSelector.cpp:31
@@ -29,1 +30,3 @@
 
+static const int WordSize = 2;
+static const int DisplacementBits = 10;
----------------
asl wrote:
> Why these are ints? And not inside namespace { ?
`WordSize` is an int because it is involved in a calculations with negative numbers and that prevents nasty signed/unsigned promotion bugs. There is no reason for `DisplacementBits` to be signed. Both of them are used only inside `isInRange` function, so I can either move them there or move them inside a namespace and keep as global constants.

================
Comment at: lib/Target/MSP430/MSP430BranchSelector.cpp:49
@@ -41,1 +48,3 @@
+  /// the beginning of each basic block.
+  SmallVector<int, 16> BlockOffsets;
 
----------------
asl wrote:
> Why this should be a member variable? It's effectively only used inside "expandBranches". Make it local there and you won't need all that error-prone BlockOffsets.clear() dance.
I can make it local inside `runOnMachineFunction` and then pass it as a reference to `measureFunction` and `expandBranches`

================
Comment at: lib/Target/MSP430/MSP430BranchSelector.cpp:79
@@ -59,1 +78,3 @@
+  int Words = DistanceInBytes / WordSize;
+  return isInt<DisplacementBits>(Words);
 }
----------------
asl wrote:
> DisplacementBits is only used here. Why should it be static const variable after all?
It was made a constant for a testing reasons, it's very hard to find a code that is so large to be split, so lowering this value was an easy way to test the code. If it is not OK, i will remove it.

================
Comment at: lib/Target/MSP430/MSP430BranchSelector.cpp:102
@@ +101,3 @@
+/// Renumber blocks and recompute all BlockOffsets, starting from BB
+void MSP430BSel::correctBlockOffsets(MachineBasicBlock *FromBB) {
+  MF->RenumberBlocks(FromBB);
----------------
asl wrote:
> This is pretty similar to measureFunction(). Please merge into a single one to reduce code duplication.
Will do.


http://reviews.llvm.org/D20162