[EXTERNAL] Re: [PATCH] D73217: [InlineCost] Add flag to allow changing the default inline cost

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 22 13:22:07 PST 2020


On Wed, Jan 22, 2020 at 1:10 PM Michael Holman <Michael.Holman at microsoft.com>
wrote:

> The new flag isn’t only a Boolean and the value of --inline-threshold is
> not ignored. The value is used here:
>
>
>
> InlineParams llvm::getInlineParams(int Threshold) {
>
> ...
>
>   if (InlineThreshold.getNumOccurrences() > 0)
>
>     Params.DefaultThreshold = InlineThreshold;
>
>   else
>
>     Params.DefaultThreshold = Threshold;
>
>
>
> If --inline-threshold is specified, this code overrides the other 2 uses
> of InlineThreshold that I changed to DefaultThreshold, so there is no
> change to the behavior of --inline-threshold either.
>

Sorry, I completely missed this use. Then it seems ok to me. But please do
add a test.


>
>
> *From:* Teresa Johnson <tejohnson at google.com>
> *Sent:* Wednesday, January 22, 2020 1:00 PM
> *To:* reviews+D73217+public+020daf5bf27953b8 at reviews.llvm.org
> *Cc:* Michael Holman <Michael.Holman at microsoft.com>; Easwaran Raman <
> eraman at google.com>; Mircea Trofin <mtrofin at google.com>; David Li <
> davidxl at google.com>; Aditya Kumar <hiraditya at msn.com>;
> haicheng at codeaurora.org; llvm-commits <llvm-commits at lists.llvm.org>
> *Subject:* [EXTERNAL] Re: [PATCH] D73217: [InlineCost] Add flag to allow
> changing the default inline cost
>
>
>
>
>
>
>
> On Wed, Jan 22, 2020 at 12:51 PM David Li via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> davidxl added a comment.
>
> ok. I see the intention.   As long as the behavior of --inline-threshold
> option is not changed (it still overrides the new option), the new option
> seems fine to me. Adding individually controlled option for size opt seems
> a good idea too.
>
>
>
> It will affect the behavior of -inline-threshold as the patch currently
> stands (the option's value is essentially ignored). That's why I'm
> suggesting an alternative to leave -inline-threshold as is, and add a new
> bool flag e.g. -inline-threshold-overrides-opt-size, probably defaulted to
> true to keep current behavior.
>
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D73217/new/
> <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD73217%2Fnew%2F&data=02%7C01%7Cmichael.holman%40microsoft.com%7C4474af52fe8a40eb37aa08d79f7e1801%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637153236229634708&sdata=ELe%2FaJEYK9acYX7z8iEp%2FaqU9ZBB3pJvAQ7tW3XqHEc%3D&reserved=0>
>
> https://reviews.llvm.org/D73217
> <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD73217&data=02%7C01%7Cmichael.holman%40microsoft.com%7C4474af52fe8a40eb37aa08d79f7e1801%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637153236229634708&sdata=ihUnpKdxBuIpNCThb%2BFymTzGDB7QuDJBuAI2sg%2FF15Q%3D&reserved=0>
>
>
>
>
>
> --
>
> Teresa Johnson |
>
>  Software Engineer |
>
>  tejohnson at google.com |
>
>
>


-- 
Teresa Johnson |  Software Engineer |  tejohnson at google.com |
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200122/31435325/attachment.html>


More information about the llvm-commits mailing list