This is an archive of the discontinued LLVM Phabricator instance.

[test] Make bt_order_by_weight in switch.ll more robust
ClosedPublic

Authored by aeubanks on Oct 27 2020, 3:52 PM.

Details

Summary
Branch weights are not represented internally linearly with the value in
the IR. In its current state the test happened to pass, but the branch
weights for 0,3,6 and 2,5,8,9 were not actually equal.

$ opt -passes='print<branch-prob>'

shows that the sum of the branch probabilities going to bb0 and bb2 were not the same.

Printing analysis results of BPI for function 'bt_order_by_weight':
---- Branch Probabilities ----
  edge entry -> bb0 probability is 0x00000003 / 0x80000000 = 0.00%
  edge entry -> bb2 probability is 0x00000004 / 0x80000000 = 0.00%

with this change:

Printing analysis results of BPI for function 'bt_order_by_weight':
---- Branch Probabilities ----
  edge entry -> bb0 probability is 0x00000004 / 0x80000000 = 0.00%
  edge entry -> bb2 probability is 0x00000004 / 0x80000000 = 0.00%

Diff Detail

Event Timeline

aeubanks created this revision.Oct 27 2020, 3:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2020, 3:52 PM
aeubanks requested review of this revision.Oct 27 2020, 3:52 PM
hans added a comment.Oct 28 2020, 12:35 AM

Thanks for fixing, but can you please elaborate a little since I'm not familiar with the details here:

Branch weights are not represented internally linearly with the value in the IR.

That's surprising. How are they represented then? In the IR, the weights for cases 0,3,6 and 2,5,8,9 both add up to 12. Did that not reflect how the weights added up internally.

Also, going over uint32_t messes with the values, and this test shouldn't be testing for that.

Is something else handling that, then? I think there was a bug here, which is exactly why the test is checking that things still work when this happens.

aeubanks updated this revision to Diff 301347.Oct 28 2020, 11:17 AM

restore large branch weights
update comments

aeubanks edited the summary of this revision. (Show Details)Oct 28 2020, 11:17 AM

Thanks for fixing, but can you please elaborate a little since I'm not familiar with the details here:

Branch weights are not represented internally linearly with the value in the IR.

That's surprising. How are they represented then? In the IR, the weights for cases 0,3,6 and 2,5,8,9 both add up to 12. Did that not reflect how the weights added up internally.

Sometimes weights have +1 added to them for practical/theoretical reasons. They are also scaled if the total branch weights overflow uint32.
But having 0 for most of the branch weights should be more robust.

Also, going over uint32_t messes with the values, and this test shouldn't be testing for that.

Is something else handling that, then? I think there was a bug here, which is exactly why the test is checking that things still work when this happens.

Restored the large branch weights.

hans accepted this revision.Oct 28 2020, 12:54 PM

lgtm thanks!

This revision is now accepted and ready to land.Oct 28 2020, 12:54 PM
This revision was landed with ongoing or failed builds.Oct 28 2020, 1:00 PM
This revision was automatically updated to reflect the committed changes.