Page MenuHomePhabricator

Support repeated machine outlining
ClosedPublic

Authored by jinlin on Dec 4 2019, 10:29 AM.

Details

Summary

The following change is to allow the machine outlining can be applied for Nth times, where N is specified by the compiler option. By default the value of N is 1. The motivation is that the repeated machine outlining can further reduce code size. Please refer to the presentation "Improving Swift Binary Size via Link Time Optimization" in LLVM Developers' Meeting in 2019.

Diff Detail

Event Timeline

jinlin created this revision.Dec 4 2019, 10:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 10:30 AM
tellenbach added a subscriber: tellenbach.

Hi,

D69446 seems to introduce something similar. Are these patches related or is this just coincidence?

jinlin added a comment.EditedDec 5 2019, 8:47 PM

Hi,

D69446 seems to introduce something similar. Are these patches related or is this just coincidence?

Thank you for pointing out this. It could be by coincidence. I have presented this in LLVM Developers' Meeting in 10/22/2019. I didn't realize someone submitted D69446 on 10/25/2019.

In D71027, I have also fixed incorrect logic in maintaining the side-effect information for outlined function at line 1295. This bug shows up during repeated machine outliner.

Please let me know if you have any questions.

Okay, thanks for clarifying.

In D71027, I have also fixed incorrect logic in maintaining the side-effect information for outlined function at line 1295. This bug shows up during repeated machine outliner.

Would it be possible to separate both patches? The fix can then be reviewed independently from repeated outlining itself.

jinlin updated this revision to Diff 232876.Dec 9 2019, 9:31 AM
jinlin added a comment.Dec 9 2019, 9:34 AM

Okay, thanks for clarifying.

In D71027, I have also fixed incorrect logic in maintaining the side-effect information for outlined function at line 1295. This bug shows up during repeated machine outliner.

Would it be possible to separate both patches? The fix can then be reviewed independently from repeated outlining itself.

Sure. I have updated the patch. Please note that the repeating machine outliner may not work correctly without fixing the logic at line 1265.

jinlin updated this revision to Diff 232899.Dec 9 2019, 10:58 AM

Sure. I have updated the patch. Please note that the repeating machine outliner may not work correctly without fixing the logic at line 1265.

Thanks! I've commented on D69446 to see if the revision is still active. So I suggest giving this some more time.

Sure. I have updated the patch. Please note that the repeating machine outliner may not work correctly without fixing the logic at line 1265.

Thanks! I've commented on D69446 to see if the revision is still active. So I suggest giving this some more time.

Is there any update for D69446? I am writing the test case for D71217. The changes D71027 have to be checked in first. Thanks.

Is there any update for D69446? I am writing the test case for D71217. The changes D71027 have to be checked in first. Thanks.

Unfortunately no update so far. If it's okay for the other reviewers (@paquette @aschwaighofer) I suggest reviewing @jinlin 's patches.

jinlin updated this revision to Diff 246034.Feb 21 2020, 4:07 PM

Update the test from IR file to MIR file.

jinlin updated this revision to Diff 248249.Mar 4 2020, 10:57 AM

Simplified the test case.

paquette added inline comments.Mar 10 2020, 2:42 PM
llvm/lib/CodeGen/MachineOutliner.cpp
1534–1536

I think that allowing the number of repeats to be 0 is unintuitive.

Would it be possible to output an error when the number of repeats is less than 1 instead of silently changing it?

1539–1541

Can you add some debug output here that says why the outliner stopped running?

E.g.

  • "Stopped outlining because I >= NumRepeats"
  • "Stopped outlining at iteration I because no changes were found"

This debug output could then be used in a separate testcase showing that repeated outlining terminates as expected.

llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
66–72

The IR instructions inside each of the IR functions here should not be necessary.

jinlin updated this revision to Diff 249551.Mar 10 2020, 10:07 PM
jinlin marked 6 inline comments as done.
jinlin added inline comments.Mar 11 2020, 8:32 AM
llvm/lib/CodeGen/MachineOutliner.cpp
1534–1536

Done.

1539–1541

Done.

llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
66–72

Please let me know how to make it work. Initially I used those instructions to generate the first iteration machine outlining functions. If I simply remove them, it will not work.

jinlin updated this revision to Diff 249651.Mar 11 2020, 9:14 AM
jinlin updated this revision to Diff 249654.Mar 11 2020, 9:27 AM
jinlin marked an inline comment as done.
jinlin added inline comments.
llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
66–72

Now I understand what you mean. I have updated the test based on your suggestions. Thank you.

tellenbach added inline comments.Mar 11 2020, 4:46 PM
llvm/lib/CodeGen/MachineOutliner.cpp
103

Could we rename this to something like machine-outline-repeat-counts or machine-outline-runs? The term outlining itself is somewhat ambigious and the prefix machine would be more consistent.

104

Same here: I suggest something like *...to apply machine outlining*

tellenbach added inline comments.Mar 11 2020, 4:56 PM
llvm/lib/CodeGen/MachineOutliner.cpp
1534

Why is this indirection necessary?

llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
2

Could you run this test with a higher number for -outline-repeat-count to verfiy that the outlining stops when no changes are found and nothing changes (beside the name of the outlined functions)?

jinlin updated this revision to Diff 249831.Mar 11 2020, 10:12 PM
jinlin marked 3 inline comments as done.
jinlin added inline comments.
llvm/lib/CodeGen/MachineOutliner.cpp
103

Done.

104

Done.

1534

I have removed this extra variable.

jinlin marked an inline comment as done.Mar 11 2020, 10:16 PM
jinlin added inline comments.
llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
2

I have updated the test case to verify that no more machine outlining is performance after the iteration is greater than 2.

LGTM, but let's wait a bit for other comments.

paquette added inline comments.Mar 13 2020, 12:08 PM
llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
2–3

I think that it would make sense to use different check prefixes for each level of outlining. That would make it easier to show explicitly what happens when you add extra iterations.

# RUN: llc -mtriple=aarch64--- -run-pass=prologepilog -run-pass=machine-outliner -machine-outline-runs=2 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix TWO-RUNS

# RUN: llc -mtriple=aarch64--- -run-pass=prologepilog -run-pass=machine-outliner -machine-outline-runs=4 -verify-machineinstrs %s -o - | FileCheck %s --check-prefix FOUR-RUNS

Also you shouldn't need --run-pass=prologepilog here.

114–116

Can you check which instructions are in these functions?

jinlin updated this revision to Diff 250310.Mar 13 2020, 2:46 PM
jinlin marked 2 inline comments as done.
jinlin added inline comments.
llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
2–3

Done.

114–116

Done.

jinlin updated this revision to Diff 250311.Mar 13 2020, 2:54 PM

Thanks for updating this!

I have a couple nits, but I think this is pretty close to being ready. :)

llvm/lib/CodeGen/MachineOutliner.cpp
1536

Grammar nit: "Expect NumRepeat for machine outlining to be greater than or equal to 1"

1540

Nit: pull I into the loop:

for (unsigned I = 0; I < NumRepeat; I++) {
 // ...
}
1542–1543

Can we have a small testcase showing that this happens?

1548–1549

Can we have a small testcase showing that this happens?

llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
140–141

Can you add a comment explaining why these functions are not expected?

jinlin updated this revision to Diff 250658.Mar 16 2020, 5:15 PM
jinlin marked 2 inline comments as done.
jinlin added inline comments.
llvm/lib/CodeGen/MachineOutliner.cpp
1536

Done.

llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir
140–141

Done.

jinlin marked 3 inline comments as done.Mar 16 2020, 5:20 PM
jinlin added inline comments.
llvm/lib/CodeGen/MachineOutliner.cpp
1540

Done.

1542–1543

The prefix FOUR-RUN shows that the machine outliner stops at 3rd iteration since no changes were found.

1548–1549

The prefix ONE-RUN shows that the machine outliner stops at 1st first iteration since NumRepeat is 1.

paquette accepted this revision.Mar 16 2020, 5:51 PM

LGTM!

This revision is now accepted and ready to land.Mar 16 2020, 5:51 PM

LGTM!

Thank you Jessica and David for reviewing my changes. Appreciate your excellent suggestions.

This revision was automatically updated to reflect the committed changes.
jinlin updated this revision to Diff 250904.Mar 17 2020, 2:31 PM
jinlin updated this revision to Diff 251122.Mar 18 2020, 10:04 AM

I have made minor changes in test case llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir.

Testing Time: 128.22s

Expected Passes    : 36105
Expected Failures  : 163
Unsupported Tests  : 336