Re: [PATCH][RFC] Come up with -flive-patching master option.

["Martin Liška" <mliska@suse.cz>]

On 11/14/18 6:54 PM, Qing Zhao wrote:
> Hi, 
> 
> 
>> On Nov 14, 2018, at 9:03 AM, Martin Liška <mliska@suse.cz> 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
>