[PATCH] D23727: [Profile] SelectInst instrumentation Support in IR-PGO

[Justin Bogner via llvm-commits]

Xinliang David Li <davidxl at google.com> writes:
> On Wed, Aug 31, 2016 at 11:28 PM, Justin Bogner <mail at justinbogner.com> wrote:
>> Xinliang David Li <davidxl at google.com> writes:
>>> On Tue, Aug 23, 2016 at 11:18 AM, Justin Bogner
>>> <mail at justinbogner.com> wrote:
>>>> David Li via llvm-commits <llvm-commits at lists.llvm.org> writes:
>>>>> Select instructions are currently ignored by PGO leading to missing
>>>>> profile data which is useful in guiding the select instruction
>>>>> lowering. This patch implements this missing feature for IRPGO.
>>>>>
>>>>> A new counter increment intrinsic is introduced (extended from
>>>>> existing increment) that also takes a step argument. It is used to
>>>>> track the number times the true value of a select instruction is
>>>>> taken. The FalseValue count is derived from TrueValue count and the
>>>>> parent BB's profile count.
>>>>
>>>> Does this really merit a new intrinsic? Why not just teach the existing
>>>> intrinsic to take a step argument?
>>>
>>> If you think this is the right thing to do, I can have a follow up
>>> patch to deprecate the old intrinsic.
>>
>> That seems like the worst of all worlds - we'd end up with an intrinsic
>> with a worse name, yet it always needs an argument that's usually 1, and
>> we'd have to carry a deprecated intrinsic for however long. What I'm
>> saying is, why not just change llvm.instrprof.increment to take one more
>> argument for the step? The bitcode upgrade should be trivial (if it
>> doesn't have enough arguments add a "1"), and updating test cases should
>> be simple enough.
>
> Actually I find keeping the existing intrinsic unchanged is better.
> Changing it means that IR produced will be slightly larger and we will
> need to update lots of tests. The Clang FE also needs to be changed to
> understand the change. On the other hand, the new intrinsic adds
> almost no additional (maintenance) overhead. Do you feel strongly
> changing this?

My concern is that having two intrinsics makes this a little harder to
understand - people need to decide which one to use, there need to be
more docs to go through, etc. This also sets a precedent for adding a
step argument, so if we wanted the value intrinsic to have a step
argument we'd add yet another intrinsic, which adds the same kind of
minor complexity.

On the other hand, the test and frontend updates are pretty simple and
mechanical, so I don't really think that would be a big deal, and the
size increase to the IR is almost negligible (it's one small number per
call).

Personally I think it's worth doing the small amount of work to just
combine this into the one intrinsic. That said, it's not *that bad* to
have two, so if you feel strongly the other way I suppose it would be
fine. In any case, the patch looks good pending whatever we decide here.