This patch implements a handling of prof branch_weights metadata for switch instructions.
New tests are created.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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:
- We appear to have the same lost profile problem for partial unswitching of branches.
- 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.
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.
- made use of SwitchInstProfBranchWeightsWrapper introduced in D62122.
- created new profmd tests instead of changing the existing tests
@asbirlea, @fedor.sergeev, @mkazantsev, @chandlerc could you please review the patch or suggest somebody who can take a look?
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. |
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. |
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. |
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
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.