On 11/14/18 6:54 PM, Qing Zhao wrote:
> Hi,
>
>
>> On Nov 14, 2018, at 9:03 AM, Martin Liška <[email protected]> wrote:
>>
>>>>
>>> Yes, you are right. I added this into my patch.
>>>
>>> I am attaching the new patch here.
>>
>> Hello.
>>
>> Please use
>> git diff HEAD~ > /tmp/patch && ~/Programming/gcc/contrib/check_GNU_style.py
>> /tmp/patch
>>
>> in order to address many formatting issues of the patch (skip the ones
>> reported in common.opt).
>
> will do and fix the style issues.
>
>>
>>>
>>>
>>> +flive-patching
>>> +Common RejectNegative Alias(flive-patching=,inline-clone) Optimization
>>> +
>>> +flive-patching=
>>> +Common Report Joined RejectNegative Enum(live_patching_level)
>>> Var(flag_live_patching) Init(LIVE_NONE) Optimization
>>> +-flive-patching=[inline-only-static|inline-clone] Control ipa
>>> optimizations to provide a
>>
>> Please use 'IPA' instead of 'ipa', similarly in documentation.
> Okay.
>>
>>> --- a/gcc/doc/invoke.texi
>>> +++ b/gcc/doc/invoke.texi
>>> @@ -411,10 +411,11 @@ Objective-C and Objective-C++ Dialects}.
>>> -fgcse-sm -fhoist-adjacent-loads -fif-conversion @gol
>>> -fif-conversion2 -findirect-inlining @gol
>>> -finline-functions -finline-functions-called-once -finline-limit=@var{n}
>>> @gol
>>> --finline-small-functions -fipa-cp -fipa-cp-clone @gol
>>> +-finline-small-functions -fipa-cp -fipa-cp-clone @gol
>>
>> This changes is probably not intended.
> No. will delete it.
>
>>
>>>
>>> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
>>> index 500f663..72e0f0f 100644
>>> --- a/gcc/flag-types.h
>>> +++ b/gcc/flag-types.h
>>> @@ -123,6 +123,14 @@ enum stack_reuse_level
>>> SR_ALL
>>> };
>>>
>>> +/* The live patching level. */
>>> +enum live_patching_level
>>> +{
>>> + LIVE_NONE = 0,
>>> + LIVE_INLINE_ONLY_STATIC,
>>> + LIVE_INLINE_CLONE
>>
>> Maybe better LIVE_PATCHING_INLINE_CLONE, without the 'PATCHING' the enum
>> values are bit unclear.
>
> Okay.
>>
>>>
>>> + /* visibility change should be excluded by !flag_whole_program
>>> + && !in_lto_p && !flag_ipa_cp_clone && !flag_ipa_sra
>>
>> You added sorry about LTO, maybe then !in_lto_p would be always true?
>
> Yes, since live-patching does not support LTO currently, !in_lto_p is always
> TRUE. that’s the reason no need for a new flag to disable visibility change.
Hi.
Ok.
>>
>>> + && !flag_partial_inlining. */
>>> + if (!opts_set->x_flag_ipa_pta)
>>> + opts->x_flag_ipa_pta = 0;
>>> + if (!opts_set->x_flag_ipa_reference)
>>> + opts->x_flag_ipa_reference = 0;
>>> + if (!opts_set->x_flag_ipa_ra)
>>> + opts->x_flag_ipa_ra = 0;
>>> + if (!opts_set->x_flag_ipa_icf)
>>> + opts->x_flag_ipa_icf = 0;
>>> + if (!opts_set->x_flag_ipa_icf_functions)
>>> + opts->x_flag_ipa_icf_functions = 0;
>>> + if (!opts_set->x_flag_ipa_icf_variables)
>>> + opts->x_flag_ipa_icf_variables = 0;
>>> + if (!opts_set->x_flag_ipa_bit_cp)
>>> + opts->x_flag_ipa_bit_cp = 0;
>>> + if (!opts_set->x_flag_ipa_vrp)
>>> + opts->x_flag_ipa_vrp = 0;
>>> + if (!opts_set->x_flag_ipa_pure_const)
>>
>> Can you please explain why you included:
>> if (!opts_set->x_flag_ipa_bit_cp)
>> opts->x_flag_ipa_bit_cp = 0;
>> if (!opts_set->x_flag_ipa_vrp)
>> opts->x_flag_ipa_vrp = 0;
>
> interprocedural bitwise constant propagation and interprocedural propagation
> of value ranges does not involve creating clones,
> and the bitwise constant and value ranges info extracted during ipa-cp phase
> are used later by other optimizations. their effect on
> impact functions are not clear at this moment. that’s the reason I think we
> need to disable these two.
>
> Martin Jambor raised this issue during our previous discussion on 10/03/2018:
> “
> I was thinking a bit more about this and recalled that not all stuff
> that IPA-CP nowadays does involves creating clones, so we have to add
> also:
> - -fno-ipa-bit-cp, and
> - -fno-ipa-vrp.
>
> These two just record info in the parameters of *callees* of functions
> from which it extracted info, without any cloning involved. Both were
> introduced in GCC 7.
>
> Thanks,
>
> Martin
> “
> and I think he is right.
Great, thanks for clarification! I forgot about that.
And please can you mention in documentation which options are disabled with
-flive-patching=*?
We usually do it, e.g. take a look at '-Os' option:
```
‘-Os’ disables the following optimization flags:
-falign-functions -falign-jumps -falign-loops
-falign-labels -freorder-blocks -freorder-blocks-algorithm=stc
-freorder-blocks-and-partition -fprefetch-loop-arrays
```
>
>
>
>> ?
>>
>>> + opts->x_flag_ipa_pure_const = 0;
>>> + /* unreachable code removal. */
>>> + /* discovery of functions/variables with no address taken. */
>>
>> ^^^ these 2 comments looks misaligned.
>
> will fix them.
>>
>>>
>>> +++ b/gcc/testsuite/gcc.dg/live-patching-1.c
>>> @@ -0,0 +1,22 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O2 -flive-patching=inline-only-static -fdump-ipa-inline"
>>> } */
>>> +
>>> +extern int sum, n, m;
>>> +
>>> +int foo (int a)
>>> +{
>>> + return a + n;
>>> +}
>>> +
>>> +static int bar (int b)
>>> +{
>>> + return b * m;
>>> +}
>>> +
>>> +int main()
>>> +{
>>> + sum = foo (m) + bar (n);
>>> + return 0;
>>> +}
>>> +
>>> +/* { dg-final { scan-ipa-dump "foo/0 function has external linkage when
>>> the user requests only inlining static for live patching" "inline" } } */
>>> -- 1.9.1
>>
>> It would be also handy to test the newly added option.
>
> not sure how to test it? any suggestion?
I would just e.g. copy test for recently added
gcc.target/i386/ipa-stack-alignment.c and replace the
option with corresponding -flive-patching option.
>
>> Please add ChangeLog entry for the patch.
>
> Okay.
That will help you to set up a skeleton:
./contrib/mklog /tmp/patch > /tmp/changelog
>
>> Have you bootstrapped the patch and run test-suite?
> did on aarch64. I will do it on x86_64 as well.
Good, please mentioned that when sending a patch next time.
One more nit, please use gcc_unreachable instead of gcc_assert (0).
Thanks for working on that,
Martin
>
> thanks.
>
> Qing
>
