This is an archive of the discontinued LLVM Phabricator instance.

[SimpleLoopUnswitch] Implement handling of prof branch_weights metadata for SwitchInst
ClosedPublic

Authored by yrouban on Apr 12 2019, 3:49 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yrouban created this revision.Apr 12 2019, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 3:49 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
reames added a subscriber: reames.Apr 12 2019, 11:40 AM

The code change looks reasonable to me, but I'd like to let Chandler confirm since he's more familiar with the code.

You definitely need a few more tests.

Two additional comments:

  1. We appear to have the same lost profile problem for partial unswitching of branches.
  2. When I wrote my early comment, I didn't realize this was sitting on top of proposed API changes. I'd suggest focusing on the earlier patches, then wan return to this one.
  1. We appear to have the same lost profile problem for partial unswitching of branches.

Yes. But I believe that the fix would be unrelated to this patch. I do not see any way to fix branch_weights for partially unswitched branch instruction other than to drop the prof branch_weights metadata.

yrouban updated this revision to Diff 195315.Apr 16 2019, 12:51 AM

added more test checks to trivial-unswitch.ll to cover all new and changed code

yrouban updated this revision to Diff 200257.May 20 2019, 5:20 AM
yrouban edited the summary of this revision. (Show Details)
yrouban set the repository for this revision to rG LLVM Github Monorepo.
  • made use of SwitchInstProfBranchWeightsWrapper introduced in D62122.
  • created new profmd tests instead of changing the existing tests
yrouban abandoned this revision.May 27 2019, 8:57 PM

@asbirlea, @fedor.sergeev, @mkazantsev, @chandlerc could you please review the patch or suggest somebody who can take a look?

yrouban reclaimed this revision.May 27 2019, 8:58 PM

I'd prefer to have Chandler approve this, since I'm not familiar with what you're trying to do here.
Some minor comments inline.

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
750 ↗(On Diff #200257)

Does this compile?

793 ↗(On Diff #200257)

I find it somewhat confusing that this is making changes both through SI and SIW. But I don't really understand the SwitchInstProfBranchWeightsWrapper, so perhaps it's WAI.

yrouban updated this revision to Diff 205507.Jun 18 2019, 10:46 PM

Rebased to the latest HEAD. Please review. Its sibling patches have been landed.

reames requested changes to this revision.Jun 20 2019, 1:46 PM
reames added inline comments.
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
602 ↗(On Diff #205507)

This bit appears unnecessarily complex. In particular, you can grab the default weight regardless of whether it's used or not later.

681 ↗(On Diff #205507)

This is not idiomatic. Please leave NewSI as it was, then define a separate updater object which wraps that.

763 ↗(On Diff #205507)

Hm, I think there's a potential reorg of the existing code which might make this easy to follow. Is there any reason we can't fully construct the switch, and then check if all successors are common? The checking early, transform late structure just seems oddly complex for no obvious reason?

llvm/test/Transforms/SimpleLoopUnswitch/basictest-profmd.ll
4 ↗(On Diff #205507)

If this doesn't unswitch, what is it testing?

llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch-profmd.ll
6 ↗(On Diff #205507)

Given the control flow, you need at least one test case where we unswitch the default only, one where we unswitch cases only, and one where we do a combination of both.

This revision now requires changes to proceed.Jun 20 2019, 1:46 PM
yrouban marked 5 inline comments as done.Jun 26 2019, 1:02 AM
yrouban added inline comments.
llvm/test/Transforms/SimpleLoopUnswitch/trivial-unswitch-profmd.ll
6 ↗(On Diff #205507)

This advice revealed one case I missed to set weight for the default switch case. There were no branch in the original code for this case.

yrouban updated this revision to Diff 206606.Jun 26 2019, 1:09 AM
yrouban marked an inline comment as done.

Addressed most comments. The main changes are the following:

  • added more cases to trivial-unswitch-profmd.ll
  • brushed up basictest-profmd.ll
  • added NewSI default case weight if the default successor was not an exit
reames accepted this revision.Jun 28 2019, 2:34 PM

LGTM. The flow through here is still confusing, but it's a lot better than it was. I'm fine seeing this landed, and then I may take a shot at an NFC to clarify what's going on.

This revision is now accepted and ready to land.Jun 28 2019, 2:34 PM
This revision was automatically updated to reflect the committed changes.