This is an archive of the discontinued LLVM Phabricator instance.

[MSP430] PR27500 CodeGen: Rework branch-select pass
ClosedPublic

Authored by pftbest on May 11 2016, 6:10 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

pftbest updated this revision to Diff 56882.May 11 2016, 6:10 AM
pftbest retitled this revision from to [MSP430] PR27500 CodeGen: Rework branch-select pass.
pftbest updated this object.
pftbest added a reviewer: asl.
pftbest added a subscriber: llvm-commits.

ping.
please review this change.

ping


llvm-commits mailing list
llvm-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

Hi! Any luck getting this diff reviewed? Is there some way I can help?

asl added inline comments.May 30 2016, 12:59 PM
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.

pftbest added inline comments.May 30 2016, 1:46 PM
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.

asl added inline comments.May 30 2016, 3:12 PM
lib/Target/MSP430/MSP430BranchSelector.cpp
31 ↗(On Diff #56882)

please move inside isInRange.

49 ↗(On Diff #56882)

This might also work, yes.

pftbest updated this revision to Diff 59006.May 30 2016, 5:11 PM

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:

  1. It happens only once per function in common case
  2. 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
  3. In extremely rare case when both successors are out of range, only then blocks are renumbered and remeasured.

So the point is:

  1. This code is not slower than what is now in the trunk
  2. But it is definitely better, because it does work and trunk is generating garbage that crashes mcu.
asl accepted this revision.Jun 26 2016, 2:28 PM
asl edited edge metadata.

LGTM. Thanks for doing this!

This revision is now accepted and ready to land.Jun 26 2016, 2:28 PM
asl added a comment.Jul 17 2016, 8:11 AM

Working on updating the patch due to some API changes.

pftbest updated this revision to Diff 67959.Aug 13 2016, 2:04 AM
pftbest edited edge metadata.

Updated diff due to API changes.

Please review this diff.

This revision was automatically updated to reflect the committed changes.