<div dir="ltr">Hmm, fair enough. I will revert and let's continue the discussion on the other thread.<div><br></div><div>Peter</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 6, 2018 at 12:03 PM, Chad Rosier <span dir="ltr"><<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<p>Hi Peter,<br>
Unfortunately, it's difficult for me to chime in on what is or is
not the right approach to solving this problem; I'm not a domain
expert for LTO. What you're suggesting does make sense to me
(i.e., adding the LTO APIs as a temporary stop gap) and I assure
you I very much want to accommodate you and your customer's
request.</p>
<p>However, after this commit if a users requests -O3 for both the
compilation and link steps with LTO that's not what they get.
While I'm not a domain expert, that does seem very non-intuitive
to me. Moreover, this commit resulted in a large number of
performance regressions across a wide suite of benchmarks. In
such scenarios, the general practice in the community has been to
revert such changes immediately and then have the necessary
discussion to move forward, not the other way around.</p>
<p>This is my request and I would still like to see this commit
reverted per community standard practices.<br>
</p>
Unfortunately, I don't have the time or resources to address this
issue myself (as I initially began investigating in D45225/D45226).
My only request is that whatever solution is decided upon that it
doesn't cause avoidable regressions.<span class="HOEnZb"><font color="#888888"><br>
<br>
Chad</font></span><div><div class="h5"><br>
<br>
<div class="m_4799521531153644796moz-cite-prefix">On 4/6/2018 1:37 PM, Peter
Collingbourne wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">On Fri, Apr 6, 2018 at 10:15 AM, Chad
Rosier <span dir="ltr"><<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<p>Hi Peter,<br>
</p>
<span> On 4/6/2018 12:25 PM, Peter
Collingbourne wrote:<br>
<blockquote type="cite">
<div dir="auto">
<div>Unfortunately reverting would regress the
user that requested this patch. They use
-plugin-opt=O0 and expect performance consistent
with setting cg opt level to default.</div>
</div>
</blockquote>
<br>
</span> Using -O0 with LTO seems like a very
odd/uncommon use case.</div>
</blockquote>
<div><br>
</div>
<div>It can be used to get program transformations that rely
on whole-program information (such as <a href="https://clang.llvm.org/docs/ControlFlowIntegrity.html" target="_blank">control flow integrity</a>)
without requesting cross-module optimizations. This is
important for users who want CFI but do not want to pay
the link time or code size cost of cross-module
optimizations.</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">Can the user use
'-mllvm -cg-opt-level=2' as an alternative solution to
override the codegen optimization level used during LTO?</div>
</blockquote>
<div><br>
</div>
<div>This flag is not present in the gold plugin, only in
llvm-lto2.</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"><span><br>
<br>
<blockquote type="cite">
<div dir="auto">
<div>
<div dir="auto"><br>
</div>
<div dir="auto">Instead of reverting can we make
a change that just sets the cg opt level to
aggressive if -plugin-opt=O3 is passed?</div>
</div>
</div>
</blockquote>
<br>
</span> I'd prefer not. IMO, this would just further
muddy the water. The '-mllvm -cg-opt-level=2'
suggestion seems like a much less intrusive
fix/workaround.</div>
</blockquote>
<div><br>
</div>
<div>Adding the flag would be going in the wrong direction
IMHO. What we should be working towards is removing the
LTO configuration option entirely. We can do that by
moving the default/aggressive selection into the LTO API
implementation so that the behaviour is consistent between
linkers. Then once the attribute is ready we can remove
that code.</div>
<div><br>
</div>
<div>Peter </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div>
<div class="m_4799521531153644796h5"><br>
<br>
<blockquote type="cite">
<div dir="auto">
<div>
<div dir="auto"><br>
</div>
<div dir="auto">Peter</div>
<br>
<br>
<div class="gmail_quote">
<div dir="ltr">On Fri, Apr 6, 2018, 09:18
Chad Rosier <<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks,
Rafael. Sound reasonable to you, Peter?<br>
<br>
<br>
On 4/6/2018 12:12 PM, Rafael Avila de
Espindola wrote:<br>
> I am OK with reverting it until we
have a better framework in place.<br>
><br>
> Cheers,<br>
> Rafael<br>
><br>
> Chad Rosier via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" rel="noreferrer" target="_blank">llvm-commits@lists.llvm.org</a>>
writes:<br>
><br>
>> Peter/Rafael,<br>
>><br>
>> Over the past few days I've been
investigating several regressions with<br>
>> our weekly LTO configuration. As
a result, I identified that the<br>
>> following regressions were due to
this commit:<br>
>><br>
>> SPEC2006/h264ref: -2.68%<br>
>> SPEC2006/libquantum: -4.36%<br>
>> SPEC2006/perlbench: -2.67%<br>
>> SPEC2017/mcf: -4.47%<br>
>> SPEC2017/perlbench: -3.82%<br>
>><br>
>> I did not test SPEC2000,
SPEC2006fp, or SPEC2017fp, but I'm seeing<br>
>> similar small regressions in our
weekly runs there as well (i.e., the<br>
>> above is just what I was able to
test last night, but isn't likely the<br>
>> only regressions). I'd like to
kindly request that this change be<br>
>> reverted until something is in
place to properly propagate the codegen<br>
>> optimization level.<br>
>><br>
>> Chad<br>
>><br>
>><br>
>> On 2/7/2018 9:41 PM, Peter
Collingbourne via llvm-commits wrote:<br>
>>> Author: pcc<br>
>>> Date: Wed Feb 7 18:41:22
2018<br>
>>> New Revision: 324557<br>
>>><br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=324557&view=rev" rel="noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=324557&view=rev</a><br>
>>> Log:<br>
>>> gold-plugin: Do not set
codegen opt level based on LTO opt level.<br>
>>><br>
>>> The LTO opt level should not
affect the codegen opt level, and indeed<br>
>>> it does not affect it in lld.
Ideally the codegen opt level should<br>
>>> be controlled by an IR-level
attribute based on the compile-time opt<br>
>>> level, but that hasn't been
implemented yet.<br>
>>><br>
>>> Differential Revision: <a href="https://reviews.llvm.org/D43040" rel="noreferrer noreferrer" target="_blank">https://reviews.llvm.org/D4304<wbr>0</a><br>
>>><br>
>>> Modified:<br>
>>>
llvm/trunk/tools/gold/gold-pl<wbr>ugin.cpp<br>
>>><br>
>>> Modified:
llvm/trunk/tools/gold/gold-plu<wbr>gin.cpp<br>
>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/gold/gold-plugin.cpp?rev=324557&r1=324556&r2=324557&view=diff" rel="noreferrer noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/tools/gold/go<wbr>ld-plugin.cpp?rev=324557&r1=32<wbr>4556&r2=324557&view=diff</a><br>
>>>
==============================<wbr>==============================<wbr>==================<br>
>>> ---
llvm/trunk/tools/gold/gold-plu<wbr>gin.cpp
(original)<br>
>>> +++
llvm/trunk/tools/gold/gold-plu<wbr>gin.cpp
Wed Feb 7 18:41:22 2018<br>
>>> @@ -727,20 +727,6 @@ static
int getOutputFileName(StringRef I<br>
>>> return FD;<br>
>>> }<br>
>>><br>
>>> -static CodeGenOpt::Level
getCGOptLevel() {<br>
>>> - switch (options::OptLevel)
{<br>
>>> - case 0:<br>
>>> - return CodeGenOpt::None;<br>
>>> - case 1:<br>
>>> - return CodeGenOpt::Less;<br>
>>> - case 2:<br>
>>> - return
CodeGenOpt::Default;<br>
>>> - case 3:<br>
>>> - return
CodeGenOpt::Aggressive;<br>
>>> - }<br>
>>> - llvm_unreachable("Invalid
optimization level");<br>
>>> -}<br>
>>> -<br>
>>> /// Parse the
thinlto_prefix_replace option into the \p
OldPrefix and<br>
>>> /// \p NewPrefix strings,
if it was specified.<br>
>>> static void
getThinLTOOldAndNewPrefix(std:<wbr>:string
&OldPrefix,<br>
>>> @@ -767,7 +753,6 @@ static
std::unique_ptr<LTO> createLTO(In<br>
>>><br>
>>> Conf.MAttrs = MAttrs;<br>
>>> Conf.RelocModel =
RelocationModel;<br>
>>> - Conf.CGOptLevel =
getCGOptLevel();<br>
>>> Conf.DisableVerify =
options::DisableVerify;<br>
>>> Conf.OptLevel =
options::OptLevel;<br>
>>> if
(options::Parallelism)<br>
>>><br>
>>><br>
>>>
______________________________<wbr>_________________<br>
>>> llvm-commits mailing list<br>
>>> <a href="mailto:llvm-commits@lists.llvm.org" rel="noreferrer" target="_blank">llvm-commits@lists.llvm.org</a><br>
>>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
>> ______________________________<wbr>_________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org" rel="noreferrer" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
<br clear="all">
<div><br>
</div>
-- <br>
<div class="m_4799521531153644796gmail_signature" data-smartmail="gmail_signature">
<div dir="ltr">--
<div>Peter</div>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div>