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

Fri Apr 6 13:58:16 PDT 2018

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
> <https://clang.llvm.org/docs/ControlFlowIntegrity.html>) 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
>>> >>> 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
>>> >>>
>>> >>> 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/go
>>> ld-plugin.cpp?rev=324557&r1=324556&r2=324557&view=diff
>>> >>> ============================================================
>>> ==================
>>> >>> --- 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
>>> >> _______________________________________________
>>> >> llvm-commits mailing list
>>> >> llvm-commits at lists.llvm.org
>>> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>>
>>
>
>
> --
> --
> Peter
>
>
>

-- 
-- 
Peter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180406/3f633e92/attachment-0001.html>