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

Fri Apr 6 14:17:51 PDT 2018

Reverted in r329458.

Peter

On Fri, Apr 6, 2018 at 1:58 PM, Peter Collingbourne <peter at pcc.me.uk> 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
>> <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
>

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