llvm dev - Sep 2017 - RFC: Use closures to delay construction of optimization remarks

Another alternative could be:

ORE.emitMissed(DEBUG_TYPE, ...) << ...

Then the first line of emitMissed does a check if it is enabled and if not
then returns a dummy stream that does nothing for operator<< (and
short-circuits all the stream operations)

On Sep 15, 2017 2:21 PM, "Adam Nemet via llvm-dev" <llvm-dev at
lists.llvm.org>
wrote:

For better readability we typically create remarks and call
OptimizationRemarkEmitter::emit unconditionally.  E.g.:

Transforms/IPO/Inliner.cpp:    ORE.emit(OptimizationRemarkMissed(DEBUG_TYPE,
"TooCostly", Call)
Transforms/IPO/Inliner.cpp-             << NV("Callee", Callee)
<< " not
inlined into "
Transforms/IPO/Inliner.cpp-             << NV("Caller", Caller)
<< "
because too costly to inline (cost="
Transforms/IPO/Inliner.cpp-             << NV("Cost",
IC.getCost())
Transforms/IPO/Inliner.cpp-             << ", threshold="
<<
NV("Threshold", IC.getThreshold()) << ")");

Then inside ORE we return right away if the remarks for the given pass is
not enabled.

This is nice and concise however there is still some overhead involved in
this if remarks are not enabled:

1. Constructing the remark object
2. Computing and inserting the strings, named-value arguments
3. Performing the comparison between the pass name and the passes enabled
by the user
4. Virtual destructor

Now that Vivek added support for asking LLVMContext for what remarks are
enabled by the user [1] we can improve this.  The idea is to allow
ORE::emit to take a closure.  This delays all this work until we know
remarks are enabled.  E.g. the above code with closure:

    ORE.emit(*[&]() {*
      *return* OptimizationRemarkMissed(DEBUG_TYPE, "TooCostly", Call)
             << NV("Callee", Callee) << " not inlined
into "
             << NV("Caller", Caller) << " because too
costly to inline
(cost="
             << NV("Cost", IC.getCost())
             << ", threshold=" <<
NV("Threshold", IC.getThreshold()) << ")"
*;*
*    }*);


I have a proof-of-concept implementation at https://reviews.llvm.org/D37921.

The main change is that since in the lambda the remark is now returned by
value, we need to preserve its type in the insertion operator.  This
requires making the insertion operator generic.  I am also curious if
people see C++ portability problems with the code.

Feedback welcome.

Adam

[1] https://reviews.llvm.org/D33514


_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170916/d9a14e1d/attachment.html>

Actually maybe something like:

if (auto &E = ORE.emitMissed(DEBUG_TYPE)) {
  E.emit(...) << ...;
}

Credit to Chandler for that (if I'm remembering it right. This is vaguely
recollected from a LLVM social conversation a long time ago about reducing
overhead of clang diagnostics that are not enabled, which sounds like the
same problem)

On Sep 16, 2017 4:39 PM, "Sean Silva" <chisophugis at gmail.com>
wrote:
> Another alternative could be:
>
> ORE.emitMissed(DEBUG_TYPE, ...) << ...
>
> Then the first line of emitMissed does a check if it is enabled and if not
> then returns a dummy stream that does nothing for operator<< (and
> short-circuits all the stream operations)
>
> On Sep 15, 2017 2:21 PM, "Adam Nemet via llvm-dev" <
> llvm-dev at lists.llvm.org> wrote:
>
> For better readability we typically create remarks and call
> OptimizationRemarkEmitter::emit unconditionally.  E.g.:
>
> Transforms/IPO/Inliner.cpp:   
ORE.emit(OptimizationRemarkMissed(DEBUG_TYPE,
> "TooCostly", Call)
> Transforms/IPO/Inliner.cpp-             << NV("Callee",
Callee) << " not
> inlined into "
> Transforms/IPO/Inliner.cpp-             << NV("Caller",
Caller) << "
> because too costly to inline (cost="
> Transforms/IPO/Inliner.cpp-             << NV("Cost",
IC.getCost())
> Transforms/IPO/Inliner.cpp-             << ", threshold="
<<
> NV("Threshold", IC.getThreshold()) << ")");
>
> Then inside ORE we return right away if the remarks for the given pass is
> not enabled.
>
> This is nice and concise however there is still some overhead involved in
> this if remarks are not enabled:
>
> 1. Constructing the remark object
> 2. Computing and inserting the strings, named-value arguments
> 3. Performing the comparison between the pass name and the passes enabled
> by the user
> 4. Virtual destructor
>
> Now that Vivek added support for asking LLVMContext for what remarks are
> enabled by the user [1] we can improve this.  The idea is to allow
> ORE::emit to take a closure.  This delays all this work until we know
> remarks are enabled.  E.g. the above code with closure:
>
>     ORE.emit(*[&]() {*
>       *return* OptimizationRemarkMissed(DEBUG_TYPE, "TooCostly",
Call)
>              << NV("Callee", Callee) << " not
inlined into "
>              << NV("Caller", Caller) << "
because too costly to inline
> (cost="
>              << NV("Cost", IC.getCost())
>              << ", threshold=" <<
NV("Threshold", IC.getThreshold()) << ")"
> *;*
> *    }*);
>
>
> I have a proof-of-concept implementation at https://reviews.llvm.org/D3
> 7921.
>
> The main change is that since in the lambda the remark is now returned by
> value, we need to preserve its type in the insertion operator.  This
> requires making the insertion operator generic.  I am also curious if
> people see C++ portability problems with the code.
>
> Feedback welcome.
>
> Adam
>
> [1] https://reviews.llvm.org/D33514
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170916/1c0006f9/attachment.html>

On 09/16/2017 06:49 PM, Sean Silva via llvm-dev wrote:
> Actually maybe something like:
>
> if (auto &E = ORE.emitMissed(DEBUG_TYPE)) {
>   E.emit(...) << ...;
> }
>
> Credit to Chandler for that (if I'm remembering it right. This is 
> vaguely recollected from a LLVM social conversation a long time ago 
> about reducing overhead of clang diagnostics that are not enabled, 
> which sounds like the same problem)
This also seems like a good option.

  -Hal
>
> On Sep 16, 2017 4:39 PM, "Sean Silva" <chisophugis at
gmail.com
> <mailto:chisophugis at gmail.com>> wrote:
>
>     Another alternative could be:
>
>     ORE.emitMissed(DEBUG_TYPE, ...) << ...
>
>     Then the first line of emitMissed does a check if it is enabled
>     and if not then returns a dummy stream that does nothing for
>     operator<< (and short-circuits all the stream operations)
>
>     On Sep 15, 2017 2:21 PM, "Adam Nemet via llvm-dev"
>     <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
>
>         For better readability we typically create remarks and call
>         OptimizationRemarkEmitter::emit unconditionally. E.g.:
>
>         Transforms/IPO/Inliner.cpp:
>          ORE.emit(OptimizationRemarkMissed(DEBUG_TYPE,
"TooCostly", Call)
>         Transforms/IPO/Inliner.cpp- << NV("Callee", Callee)
<< " not
>         inlined into "
>         Transforms/IPO/Inliner.cpp- << NV("Caller", Caller)
<< "
>         because too costly to inline (cost="
>         Transforms/IPO/Inliner.cpp- << NV("Cost",
IC.getCost())
>         Transforms/IPO/Inliner.cpp- << ", threshold="
<<
>         NV("Threshold", IC.getThreshold()) <<
")");
>
>         Then inside ORE we return right away if the remarks for the
>         given pass is not enabled.
>
>         This is nice and concise however there is still some overhead
>         involved in this if remarks are not enabled:
>
>         1. Constructing the remark object
>         2. Computing and inserting the strings, named-value arguments
>         3. Performing the comparison between the pass name and the
>         passes enabled by the user
>         4. Virtual destructor
>
>         Now that Vivek added support for asking LLVMContext for what
>         remarks are enabled by the user [1] we can improve this.  The
>         idea is to allow ORE::emit to take a closure.  This delays all
>         this work until we know remarks are enabled.  E.g. the above
>         code with closure:
>
>             ORE.emit(*[&]() {*
>         *return* OptimizationRemarkMissed(DEBUG_TYPE,
"TooCostly", Call)
>                      << NV("Callee", Callee) <<
" not inlined into "
>                      << NV("Caller", Caller) <<
" because too costly
>         to inline (cost="
>                      << NV("Cost", IC.getCost())
>                      << ", threshold=" <<
NV("Threshold",
>         IC.getThreshold()) << ")"*;*
>         *    }*);
>
>
>         I have a proof-of-concept implementation at
>         https://reviews.llvm.org/D37921
<https://reviews.llvm.org/D37921>.
>         The main change is that since in the lambda the remark is now
>         returned by value, we need to preserve its type in the
>         insertion operator. This requires making the
>         insertion operator generic.  I am also curious if people see
>         C++ portability problems with the code.
>
>         Feedback welcome.
>
>         Adam
>
>         [1] https://reviews.llvm.org/D33514
>         <https://reviews.llvm.org/D33514>
>
>
>         _______________________________________________
>         LLVM Developers mailing list
>         llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>         http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>         <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170916/d6c80da7/attachment.html>

> On Sep 16, 2017, at 4:49 PM, Sean Silva <chisophugis at gmail.com>
wrote:
> 
> Actually maybe something like:
> 
> if (auto &E = ORE.emitMissed(DEBUG_TYPE)) {
>   E.emit(...) << ...;
> }
Well, the point of this interface was exactly to avoid writing a conditional. 
If you’re willing to use a conditional you can already write this:

if (ORE.allowExtraAnalysis(DEBUG_TYPE))
  ORE.emit(OptimizationRemark(…) << …;

But again the point was to hide all this.  I find the closure-based interface
more concise and easier to identify visually.  One reason is that the block is
contained within ORE.emit(…).  I just wish you could omit the return in a lambda
as in Python.

Adam
> 
> Credit to Chandler for that (if I'm remembering it right. This is
vaguely recollected from a LLVM social conversation a long time ago about
reducing overhead of clang diagnostics that are not enabled, which sounds like
the same problem)
> 
> On Sep 16, 2017 4:39 PM, "Sean Silva" <chisophugis at
gmail.com <mailto:chisophugis at gmail.com>> wrote:
> Another alternative could be:
> 
> ORE.emitMissed(DEBUG_TYPE, ...) << ...
> 
> Then the first line of emitMissed does a check if it is enabled and if not
then returns a dummy stream that does nothing for operator<< (and
short-circuits all the stream operations)
> 
> On Sep 15, 2017 2:21 PM, "Adam Nemet via llvm-dev" <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> For better readability we typically create remarks and call
OptimizationRemarkEmitter::emit unconditionally.  E.g.:
> 
> Transforms/IPO/Inliner.cpp:   
ORE.emit(OptimizationRemarkMissed(DEBUG_TYPE, "TooCostly", Call)
> Transforms/IPO/Inliner.cpp-             << NV("Callee",
Callee) << " not inlined into "
> Transforms/IPO/Inliner.cpp-             << NV("Caller",
Caller) << " because too costly to inline (cost="
> Transforms/IPO/Inliner.cpp-             << NV("Cost",
IC.getCost())
> Transforms/IPO/Inliner.cpp-             << ", threshold="
<< NV("Threshold", IC.getThreshold()) << ")");
> 
> Then inside ORE we return right away if the remarks for the given pass is
not enabled.
> 
> This is nice and concise however there is still some overhead involved in
this if remarks are not enabled:
> 
> 1. Constructing the remark object
> 2. Computing and inserting the strings, named-value arguments
> 3. Performing the comparison between the pass name and the passes enabled
by the user
> 4. Virtual destructor
> 
> Now that Vivek added support for asking LLVMContext for what remarks are
enabled by the user [1] we can improve this.  The idea is to allow ORE::emit to
take a closure.  This delays all this work until we know remarks are enabled. 
E.g. the above code with closure:
> 
>     ORE.emit([&]() {
>       return OptimizationRemarkMissed(DEBUG_TYPE, "TooCostly",
Call)
>              << NV("Callee", Callee) << " not
inlined into "
>              << NV("Caller", Caller) << "
because too costly to inline (cost="
>              << NV("Cost", IC.getCost())
>              << ", threshold=" <<
NV("Threshold", IC.getThreshold()) << ")";
>     });
> 
> 
> I have a proof-of-concept implementation at https://reviews.llvm.org/D37921
<https://reviews.llvm.org/D37921>.
>  
> The main change is that since in the lambda the remark is now returned by
value, we need to preserve its type in the insertion operator.  This requires
making the insertion operator generic.  I am also curious if people see C++
portability problems with the code.
> 
> Feedback welcome.
> 
> Adam
> 
> [1] https://reviews.llvm.org/D33514 <https://reviews.llvm.org/D33514>
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170916/ddb9df60/attachment.html>