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

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/5b678449/attachment.html>

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.

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/ed3ac775/attachment.html>

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>