[llvm] r324557 - gold-plugin: Do not set codegen opt level based on LTO opt level.

Sat Apr 7 14:39:51 PDT 2018

Thank you for your understanding, Peter.

On 2018-04-06 16:58, Peter Collingbourne wrote:
> Hmm, fair enough. I will revert and let's continue the discussion on
> the other thread.
> 
> Peter
> 
> On Fri, Apr 6, 2018 at 12:03 PM, Chad Rosier <mcrosier at codeaurora.org>
> wrote:
> 
>> Hi Peter,
>> 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.
>> 
>> 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.
>> 
>> This is my request and I would still like to see this commit
>> reverted per community standard practices.
>> 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.
>> 
>> Chad
>> 
>> On 4/6/2018 1:37 PM, Peter Collingbourne wrote:
>> 
>> On Fri, Apr 6, 2018 at 10:15 AM, Chad Rosier
>> <mcrosier at codeaurora.org> wrote:
>> 
>> Hi Peter,
>> On 4/6/2018 12:25 PM, Peter Collingbourne wrote:
>> 
>> 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.
>> Using -O0 with LTO seems like a very odd/uncommon use case.
> 
> It can be used to get program transformations that rely on
> whole-program information (such as control flow integrity [5]) 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.
> 
>> Can the user use '-mllvm -cg-opt-level=2' as an alternative solution
>> to override the codegen optimization level used during LTO?
> 
> This flag is not present in the gold plugin, only in llvm-lto2.
> 
>>> Instead of reverting can we make a change that just sets the cg
>>> opt level to aggressive if -plugin-opt=O3 is passed?
>> 
>> 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.
> 
> 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.
> 
> Peter
> 
>> Peter
>> 
>> On Fri, Apr 6, 2018, 09:18 Chad Rosier <mcrosier at codeaurora.org>
>> wrote:
>> Thanks, Rafael.  Sound reasonable to you, Peter?
>> 
>> On 4/6/2018 12:12 PM, Rafael Avila de Espindola wrote:
>>> I am OK with reverting it until we have a better framework in
>> place.
>>> 
>>> Cheers,
>>> Rafael
>>> 
>>> Chad Rosier via llvm-commits <llvm-commits at lists.llvm.org> writes:
>>> 
>>>> Peter/Rafael,
>>>> 
>>>> Over the past few days I've been investigating several
>> regressions with
>>>> our weekly LTO configuration.  As a result, I identified that the
>>>> following regressions were due to this commit:
>>>> 
>>>> SPEC2006/h264ref: -2.68%
>>>> SPEC2006/libquantum: -4.36%
>>>> SPEC2006/perlbench: -2.67%
>>>> SPEC2017/mcf: -4.47%
>>>> SPEC2017/perlbench: -3.82%
>>>> 
>>>> I did not test SPEC2000, SPEC2006fp, or SPEC2017fp, but I'm
>> seeing
>>>> similar small regressions in our weekly runs there as well (i.e.,
>> the
>>>> above is just what I was able to test last night, but isn't
>> likely the
>>>> only regressions).  I'd like to kindly request that this change
>> be
>>>> reverted until something is in place to properly propagate the
>> codegen
>>>> optimization level.
>>>> 
>>>> Chad
>>>> 
>>>> 
>>>> On 2/7/2018 9:41 PM, Peter Collingbourne via llvm-commits wrote:
>>>>> Author: pcc
>>>>> Date: Wed Feb  7 18:41:22 2018
>>>>> New Revision: 324557
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=324557&view=rev [1]
>>>>> Log:
>>>>> gold-plugin: Do not set codegen opt level based on LTO opt
>> level.
>>>>> 
>>>>> The LTO opt level should not affect the codegen opt level, and
>> indeed
>>>>> it does not affect it in lld. Ideally the codegen opt level
>> should
>>>>> be controlled by an IR-level attribute based on the compile-time
>> opt
>>>>> level, but that hasn't been implemented yet.
>>>>> 
>>>>> Differential Revision: https://reviews.llvm.org/D43040 [2]
>>>>> 
>>>>> Modified:
>>>>> llvm/trunk/tools/gold/gold-plugin.cpp
>>>>> 
>>>>> Modified: llvm/trunk/tools/gold/gold-plugin.cpp
>>>>> URL:
>> 
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/gold/gold-plugin.cpp?rev=324557&r1=324556&r2=324557&view=diff
>> [3]
>>>>> 
>> 
> ==============================================================================
>>>>> --- llvm/trunk/tools/gold/gold-plugin.cpp (original)
>>>>> +++ llvm/trunk/tools/gold/gold-plugin.cpp Wed Feb  7 18:41:22
>> 2018
>>>>> @@ -727,20 +727,6 @@ static int getOutputFileName(StringRef I
>>>>> return FD;
>>>>> }
>>>>> 
>>>>> -static CodeGenOpt::Level getCGOptLevel() {
>>>>> -  switch (options::OptLevel) {
>>>>> -  case 0:
>>>>> -    return CodeGenOpt::None;
>>>>> -  case 1:
>>>>> -    return CodeGenOpt::Less;
>>>>> -  case 2:
>>>>> -    return CodeGenOpt::Default;
>>>>> -  case 3:
>>>>> -    return CodeGenOpt::Aggressive;
>>>>> -  }
>>>>> -  llvm_unreachable("Invalid optimization level");
>>>>> -}
>>>>> -
>>>>> /// Parse the thinlto_prefix_replace option into the \p
>> OldPrefix and
>>>>> /// \p NewPrefix strings, if it was specified.
>>>>> static void getThinLTOOldAndNewPrefix(std::string &OldPrefix,
>>>>> @@ -767,7 +753,6 @@ static std::unique_ptr<LTO> createLTO(In
>>>>> 
>>>>> Conf.MAttrs = MAttrs;
>>>>> Conf.RelocModel = RelocationModel;
>>>>> -  Conf.CGOptLevel = getCGOptLevel();
>>>>> Conf.DisableVerify = options::DisableVerify;
>>>>> Conf.OptLevel = options::OptLevel;
>>>>> if (options::Parallelism)
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits [4]
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits [4]
> 
>  --
> 
> --
> Peter
> 
> --
> 
> --
> Peter
> 
> Links:
> ------
> [1] http://llvm.org/viewvc/llvm-project?rev=324557&view=rev
> [2] https://reviews.llvm.org/D43040
> [3]
> http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/gold/gold-plugin.cpp?rev=324557&r1=324556&r2=324557&view=diff
> [4] http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> [5] https://clang.llvm.org/docs/ControlFlowIntegrity.html