Branch instructions created during this pass now have a machine basic block as a target instead of a fixed offset. If there is no layout successor, the current MBB is split to create one. This fixes 27500.
Branch distance should be checked in words instead of a bytes, according to documentation. This prevents branch expansion from triggering too early.
Vector of block offsets is now used to calculate distances between blocks, this is more efficient when branch expansion is not needed, which is a common case.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
ping
llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
lib/Target/MSP430/MSP430BranchSelector.cpp | ||
---|---|---|
31 ↗ | (On Diff #56882) | Why these are ints? And not inside namespace { ? |
49 ↗ | (On Diff #56882) | 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. |
79 ↗ | (On Diff #56882) | DisplacementBits is only used here. Why should it be static const variable after all? |
102 ↗ | (On Diff #56882) | This is pretty similar to measureFunction(). Please merge into a single one to reduce code duplication. |
lib/Target/MSP430/MSP430BranchSelector.cpp | ||
---|---|---|
31 ↗ | (On Diff #56882) | 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. |
49 ↗ | (On Diff #56882) | I can make it local inside runOnMachineFunction and then pass it as a reference to measureFunction and expandBranches |
79 ↗ | (On Diff #56882) | 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. |
102 ↗ | (On Diff #56882) | Will do. |
Constants are moved inside isInRange function, BlockOffsets is now local and is passed as a reference to other functions, correctBlockOffsets function is merged with measureFunction.
My thoughts on block renumbering:
- It happens only once per function in common case
- In a rare case when we need some branches to be expanded, block offsets are updated manually in the loop, so blocks are not renumbered
- In extremely rare case when both successors are out of range, only then blocks are renumbered and remeasured.
So the point is:
- This code is not slower than what is now in the trunk
- But it is definitely better, because it does work and trunk is generating garbage that crashes mcu.