[SILOptimizer] Added MandatoryCombiner. #26913
Conversation
|
@swift-ci Please smoke test |
nate-chandler
force-pushed
the
mandatory-combiner
branch
2 times, most recently
from
f0c28d3 to
1a506b4
Compare
|
I just realized for a second a bit of your confusion. When I said the "loop optimization", I meant the iterators. |
nate-chandler
force-pushed
the
mandatory-combiner
branch
4 times, most recently
from
d73de89 to
14595c0
Compare
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
|
@swift-ci smoke test |
1 similar comment
|
@swift-ci smoke test |
|
@swift-ci smoke test linux |
nate-chandler
force-pushed
the
mandatory-combiner
branch
from
14595c0 to
ca1aff3
Compare
|
@swift-ci smoke test |
1 similar comment
|
@swift-ci smoke test |
| // See https://swift.org/LICENSE.txt for license information | ||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
| // | ||
| //===----------------------------------------------------------------------===// |
There was a problem hiding this comment.
I think we should establish the practice of adding file-level comments to new passes. What is the intention of the pass? Where is it expected and/or valid to run in the pipeline? Does it need to run before diagnostics? Or is it an optional Onone optimization? What are the pass dependencies--can other passes interfere with this, or is this helpful to run before certain passes?
There was a problem hiding this comment.
How would something like this be?
///
/// \file
/// Defines the MandatoryCombiner function transform. The pass contains basic
/// instruction combines to be performed at the begining of both the Onone and
/// also the performance pass pipelines, after the diagnostics passes have been
/// run. It is intended to be run before and to be independent of other
/// transforms.
///
//===----------------------------------------------------------------------===//
There was a problem hiding this comment.
Can you put a newline after \file.
Also can you put in some prose that says something along the lines of: "The intention of this pass is to be a place for mandatory peepholes that are not needed for diagnostics. Please put any such peepholes here instead of in the diagnostic passes".
| @@ -312,6 +312,8 @@ PASS(SerializeSILPass, "serialize-sil", | |||
| PASS(YieldOnceCheck, "yield-once-check", | |||
| "Check correct usage of yields in yield-once coroutines") | |||
| PASS(OSLogOptimization, "os-log-optimization", "Optimize os log calls") | |||
| PASS(MandatoryCombiner, "mandatory-combiner", | |||
There was a problem hiding this comment.
I noticed that there is a new line at the end of this line. Did you run this code through git-clang-format? (TBH, I forgot if git-clang-format handles .def files, but I think it does). If you did and my memory is wrong, can you get rid of the newline?
If you don't know how to setup git-clang-format/etc I can show you.
There was a problem hiding this comment.
Yea git-clang-format doesn't touch the .def. It does find some things though to fix. When I locally read your code and run git-clang-format on this I see:
diff --git a/lib/SILOptimizer/Mandatory/MandatoryCombiner.cpp b/lib/SILOptimizer/Mandatory/MandatoryCombiner.cpp
index feffce765ae..6c916b28d43 100644
--- a/lib/SILOptimizer/Mandatory/MandatoryCombiner.cpp
+++ b/lib/SILOptimizer/Mandatory/MandatoryCombiner.cpp
@@ -22,7 +22,7 @@
using namespace swift;
-/// \returns whether all the values are of trivial type in the provided
+/// \returns whether all the values are of trivial type in the provided
/// function.
template <typename Values>
static bool areAllValuesTrivial(Values values, SILFunction &function) {
@@ -159,7 +159,6 @@ struct MandatoryCombiner : SILFunctionTransform {
}
}
};
-
} // end anonymous namespace
SILTransform *swift::createMandatoryCombiner() {
There was a problem hiding this comment.
Getting rid of the newline results in a line with 84 characters. Is that desirable?
There was a problem hiding this comment.
Whitespace is fixed.
nate-chandler
force-pushed
the
mandatory-combiner
branch
2 times, most recently
from
c084d38 to
befe56f
Compare
|
@swift-ci smoke test |
nate-chandler
force-pushed
the
mandatory-combiner
branch
from
befe56f to
e2c8f91
Compare
|
@swift-ci please smoke test |
1 similar comment
|
@swift-ci please smoke test |
|
Nice. This LGTM. |
Minimal commit to get the mandatory combiner rolling. At the moment, the mandatory combiner only processes partial apply instructions, attempting both to replace their applies with applies of their underlying function refs and also to eliminate them altogether. That processing is only done if all the arguments to the apply and to the partial apply are trivial. For now, the pass is not part of any pipeline.
nate-chandler
force-pushed
the
mandatory-combiner
branch
from
e2c8f91 to
bed890c
Compare
|
@swift-ci please smoke test and merge |
1 similar comment
|
@swift-ci please smoke test and merge |
Minimal commit to get the mandatory combiner rolling. At the moment, the mandatory combiner only processes partial apply instructions, attempting both to replace their applies with applies of their underlying function refs and also to eliminate them altogether. That processing is only done if all the arguments to the apply and to the partial apply are trivial.
For now, the pass is not part of any pipeline.