llvm dev - Jun 2016 - The state of IRPGO (3 remaining work items)

On Mon, Jun 27, 2016 at 2:53 PM Xinliang David Li <davidxl at google.com>
wrote:
> There is some misunderstanding about the intention of this flag.  The
> purpose of the flag is not to turn on profile instrumentation (which
> already has -fprofile-instr-generate or -fprofile-generate for it), but to
> select which instrumentors to use for PGO (IR or FE).  I prefer fewer flags
> too, but sharing flags for completely different purpose does not seem like
> the right thing to do.
>
>
Ah, right. It does seem like I'm misunderstanding the intention for that
flag.

FWIW, I think consolidating the flags can happen later on, when we have a
better idea of how the different instrumentation pieces fit together. Right
now IIUC the only other thing that might count as an alternate
instrumentation implementation is XRay (and I'm not even sure they're
mutually exclusive either, i.e. both the profiling and XRay instrumentation
should be able to live together in the same binary).

So if it's just the two that will be interacting, then sharing flags
doesn't seem worth it. But if I'm missing another, then the case for
consolidating the flags becomes stronger.

Cheers

> David
>
>
> On Sun, Jun 26, 2016 at 9:39 PM, Dean Michael Berris <dberris at
google.com>
> wrote:
>
>> On Fri, Jun 24, 2016 at 1:44 AM Eric Christopher via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>>
>>>
>>> On Thu, Jun 2, 2016, 6:41 PM Xinliang David Li via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>>
>>>> Sounds fine to me, though I am not a fan of using unstable in
the
>>>> option. I think a more meaningful way (that capture the essence
of the
>>>> difference) is the following naming:
>>>> 1) FEPGO:  -fprofile-instr-generate=source or
>>>> -fprofile-instr-generate=region
>>>> 2) IR: -fprofile-instr-generate=cfg or
-fprofile-instr-generate=graph
>>>>
>>>> Also since -fprofile-instr-generate= form is already used to
specify
>>>> raw profile path, we may need a different driver option.
Alternatives
>>>> include
>>>> 1) -fprofile-instrument=<...> -- this maps directly to
the cc1 option
>>>> we have today
>>>> or
>>>> 2) -fpgo-instr=<> -- suggested by Fred or
>>>> 3) -fpgo-instr-method=<...>
>>>>
>>>
>>> Random bikeshedding. I like fprofile-instrument because it merges a
lot
>>> of similar ideas behind instrumenting - and oddly enough what I was
>>> suggesting in the x-ray thread before seeing this.
>>>
>>>
>> +1 to -fprofile-instrument=... (as someone working on the XRay stuff,
I'd
>> much rather have less flags, and consolidate a lot of these similar
things
>> into a more inclusive flag).
>>
>> I would even make it shorter, and say -finstrument={profile-...,
>> xray-...} so we can have multiple "namespaced" values for
-finstrument=.
>>
>> Just my A$0.02.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160627/17b1a42c/attachment-0001.html>

On Sun, Jun 26, 2016 at 10:21 PM, Dean Michael Berris <dberris at
google.com>
wrote:
>
>
> On Mon, Jun 27, 2016 at 2:53 PM Xinliang David Li <davidxl at
google.com>
> wrote:
>
>> There is some misunderstanding about the intention of this flag.  The
>> purpose of the flag is not to turn on profile instrumentation (which
>> already has -fprofile-instr-generate or -fprofile-generate for it), but
to
>> select which instrumentors to use for PGO (IR or FE).  I prefer fewer
flags
>> too, but sharing flags for completely different purpose does not seem
like
>> the right thing to do.
>>
>>
> Ah, right. It does seem like I'm misunderstanding the intention for
that
> flag.
>
> FWIW, I think consolidating the flags can happen later on, when we have a
> better idea of how the different instrumentation pieces fit together.
>

I hope so :)   Note that PGO related instrumentation is not entirely the
same as general profiling instrumentations. The former should also have a
corresponding profile-use component in the compiler.  Current PGO
instrumentation has edge/block profiling and a value profiler component.
The FE based edge/block profiler is also shared with coverage testing.


> Right now IIUC the only other thing that might count as an alternate
> instrumentation implementation is XRay (and I'm not even sure
they're
> mutually exclusive either, i.e. both the profiling and XRay instrumentation
> should be able to live together in the same binary).
>
Sanitizers (asan, tsan, ubsan, msan) are all program instrumenters. Asan
also supports a mode for coverage testing. Profile related, we also have
esan (efficiency sanitizer) under development.  The sanitizer options of
course are unified in its own category.

David




> So if it's just the two that will be interacting, then sharing flags
> doesn't seem worth it. But if I'm missing another, then the case
for
> consolidating the flags becomes stronger.
>
> Cheers
>
>
>> David
>>
>>
>> On Sun, Jun 26, 2016 at 9:39 PM, Dean Michael Berris <dberris at
google.com>
>> wrote:
>>
>>> On Fri, Jun 24, 2016 at 1:44 AM Eric Christopher via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>>
>>>>
>>>> On Thu, Jun 2, 2016, 6:41 PM Xinliang David Li via llvm-dev
<
>>>> llvm-dev at lists.llvm.org> wrote:
>>>>
>>>>>
>>>>> Sounds fine to me, though I am not a fan of using unstable
in the
>>>>> option. I think a more meaningful way (that capture the
essence of the
>>>>> difference) is the following naming:
>>>>> 1) FEPGO:  -fprofile-instr-generate=source or
>>>>> -fprofile-instr-generate=region
>>>>> 2) IR: -fprofile-instr-generate=cfg or
-fprofile-instr-generate=graph
>>>>>
>>>>> Also since -fprofile-instr-generate= form is already used
to specify
>>>>> raw profile path, we may need a different driver option.
Alternatives
>>>>> include
>>>>> 1) -fprofile-instrument=<...> -- this maps directly
to the cc1 option
>>>>> we have today
>>>>> or
>>>>> 2) -fpgo-instr=<> -- suggested by Fred or
>>>>> 3) -fpgo-instr-method=<...>
>>>>>
>>>>
>>>> Random bikeshedding. I like fprofile-instrument because it
merges a lot
>>>> of similar ideas behind instrumenting - and oddly enough what I
was
>>>> suggesting in the x-ray thread before seeing this.
>>>>
>>>>
>>> +1 to -fprofile-instrument=... (as someone working on the XRay
stuff,
>>> I'd much rather have less flags, and consolidate a lot of these
similar
>>> things into a more inclusive flag).
>>>
>>> I would even make it shorter, and say -finstrument={profile-...,
>>> xray-...} so we can have multiple "namespaced" values for
-finstrument=.
>>>
>>> Just my A$0.02.
>>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160626/0cfcc922/attachment.html>

On Sun, Jun 26, 2016 at 11:04 PM Xinliang David Li <davidxl at google.com>
wrote:
> On Sun, Jun 26, 2016 at 10:21 PM, Dean Michael Berris <dberris at
google.com>
> wrote:
>
>>
>>
>> On Mon, Jun 27, 2016 at 2:53 PM Xinliang David Li <davidxl at
google.com>
>> wrote:
>>
>>> There is some misunderstanding about the intention of this flag. 
The
>>> purpose of the flag is not to turn on profile instrumentation
(which
>>> already has -fprofile-instr-generate or -fprofile-generate for it),
but to
>>> select which instrumentors to use for PGO (IR or FE).  I prefer
fewer flags
>>> too, but sharing flags for completely different purpose does not
seem like
>>> the right thing to do.
>>>
>>>
>> Ah, right. It does seem like I'm misunderstanding the intention for
that
>> flag.
>>
>> FWIW, I think consolidating the flags can happen later on, when we have
a
>> better idea of how the different instrumentation pieces fit together.
>>
>
>
> I hope so :)   Note that PGO related instrumentation is not entirely the
> same as general profiling instrumentations. The former should also have a
> corresponding profile-use component in the compiler.  Current PGO
> instrumentation has edge/block profiling and a value profiler component.
> The FE based edge/block profiler is also shared with coverage testing.
>
>
>
>> Right now IIUC the only other thing that might count as an alternate
>> instrumentation implementation is XRay (and I'm not even sure
they're
>> mutually exclusive either, i.e. both the profiling and XRay
instrumentation
>> should be able to live together in the same binary).
>>
>
> Sanitizers (asan, tsan, ubsan, msan) are all program instrumenters. Asan
> also supports a mode for coverage testing. Profile related, we also have
> esan (efficiency sanitizer) under development.  The sanitizer options of
> course are unified in its own category.
>
>
Agreed. This all makes sense. Thanks David :)

-eric

> David
>
>
>
>
>
>> So if it's just the two that will be interacting, then sharing
flags
>> doesn't seem worth it. But if I'm missing another, then the
case for
>> consolidating the flags becomes stronger.
>>
>> Cheers
>>
>>
>>> David
>>>
>>>
>>> On Sun, Jun 26, 2016 at 9:39 PM, Dean Michael Berris <dberris at
google.com
>>> > wrote:
>>>
>>>> On Fri, Jun 24, 2016 at 1:44 AM Eric Christopher via llvm-dev
<
>>>> llvm-dev at lists.llvm.org> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, Jun 2, 2016, 6:41 PM Xinliang David Li via llvm-dev
<
>>>>> llvm-dev at lists.llvm.org> wrote:
>>>>>
>>>>>>
>>>>>> Sounds fine to me, though I am not a fan of using
unstable in the
>>>>>> option. I think a more meaningful way (that capture the
essence of the
>>>>>> difference) is the following naming:
>>>>>> 1) FEPGO:  -fprofile-instr-generate=source or
>>>>>> -fprofile-instr-generate=region
>>>>>> 2) IR: -fprofile-instr-generate=cfg or
-fprofile-instr-generate=graph
>>>>>>
>>>>>> Also since -fprofile-instr-generate= form is already
used to specify
>>>>>> raw profile path, we may need a different driver
option. Alternatives
>>>>>> include
>>>>>> 1) -fprofile-instrument=<...> -- this maps
directly to the cc1 option
>>>>>> we have today
>>>>>> or
>>>>>> 2) -fpgo-instr=<> -- suggested by Fred or
>>>>>> 3) -fpgo-instr-method=<...>
>>>>>>
>>>>>
>>>>> Random bikeshedding. I like fprofile-instrument because it
merges a
>>>>> lot of similar ideas behind instrumenting - and oddly
enough what I was
>>>>> suggesting in the x-ray thread before seeing this.
>>>>>
>>>>>
>>>> +1 to -fprofile-instrument=... (as someone working on the XRay
stuff,
>>>> I'd much rather have less flags, and consolidate a lot of
these similar
>>>> things into a more inclusive flag).
>>>>
>>>> I would even make it shorter, and say
-finstrument={profile-...,
>>>> xray-...} so we can have multiple "namespaced" values
for -finstrument=.
>>>>
>>>> Just my A$0.02.
>>>>
>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20160627/112c097a/attachment.html>