[llvm-dev] Revisiting/refining the definition of optnone with interprocedural transformations

[Johannes Doerfert via llvm-dev]

On 4/22/21 6:00 PM, paul.robinson at sony.com wrote:
> Hi Johannes,
>
> I've taken some time to try to understand your viewpoint,
> and I will give some more of the history as I remember it;
> hopefully that will help.  And a suggestion at the end.
>
>> The point is, I never suggested to change the meaning of
>> `optnone` (in IR). I argue to change the requirement for
>> it to always go with `noinline`. `optnone` itself is not
>> changed, you get exactly the same behavior you got before,
>> and `noinline` is also not changed. They are simply not
>> required to go together.
> Okay.
>
> I can understand looking at the as-implemented handling of
> 'optnone' and taking that as the intended meaning. So from that
> perspective, lacking the history, it does look to you like you are
> not suggesting a change in its meaning.
>
> Actually, removing the requirement to tie them together *is*
> something that I would consider a semantic change, will require an
> update to the LangRef and verifier, and so on.  This would be more
> obvious if we had originally done either of two things with the
> same net semantic effect as we have now:
> - make optnone *imply* noinline (like 'naked' does IIUC) instead
>    of *requiring* it.
> - make the inliner check for optnone on the callee, instead of
>    simply tying the two attributes together.
>
> I hope this helps explain why I see optnone+noinline as simply
> two parts of one unified feature.
>
> History:
>
> The meaning of 'optnone' was always intended as, don't optimize,
> in as many ways as we can manage.  I'd rather have not had the
> coupling with noinline, but it was the best way forward at the
> time to achieve the effect we needed.  I spent too many months of
> my life getting this accepted at all...
> Maybe my definition of optnone in the LangRef is inadequate; but
> I am quite sure I understand the original intent.
>
> Which includes this:
>
> When it comes to the interaction of optnone and inlining, there
> were of course four cases to consider: caller and callee, and each
> might or might not have optnone.
>
> 1) caller - N, callee - N
> 2) caller - N, callee - Y
> 3) caller - Y, callee - N
> 4) caller - Y, callee - Y
>
> 1) normal case. Inlining and other opts are okay.
> 2) callee is optnone, so we didn't want it inlined and optimized.
> 3) caller is optnone, so no inlining happens.
> 4) caller is optnone, so no inlining happens.
>
> Cases 3/4 are handled by checking for optnone in the inliner pass,
> just like a hundred other passes do.  This is boilerplate and was
> added in bulk to all those passes.  Having it all be boilerplate
> was important in getting the reviews accepted.
>
> Case 2 was handled by coupling optnone to noinline.  As I said,
> this was a practical thing done at the time, not because we ever
> intended the semantics of optnone to mean anything else.  It got
> the overall feature accepted, which was the key thing for Sony.
>
> So, what we implemented achieved the effect we wanted, even if
> that implementation didn't mean the new attribute *by itself* had
> exactly all the effects we wanted.
>
> Yes, decoupling optnone (as implemented) from noinline would allow
> case 2 to inline bar into foo, and optimize the result, if that
> somehow seems desirable.  But, that result is very much against
> the original intent of optnone, and is why I have been giving you
> such a hard time about this.
>
> I will continue to insist that something called 'optnone' cannot
> properly fail to apply to all instances, inlined or not.  But if
> we're willing to rename the attribute, that problem is solved.
>
>
> I'm assuming there would be resistance to doing either of the two
> things I mentioned at the top (make optnone *imply* noinline, or
> modify the inliner to check for optnone on the callee) as that
> doesn't seem to be the direction people are moving in.
>
> So the suggestion is:
> - reword the definition of optnone to be something that would be
>    better named "nolocalopt";
> - ideally, actually rename the attribute (because I still say
>    that "none" does not mean "unless we've inlined it");
> - and yes, if you must, decouple it from noinline and remove that
>    paragraph from the LangRef description.
>
> Clang will still pass both IR attributes, so end users won't see
> any feature regressions.  David can add 'noipa' and we can make
> Clang pass that as well.

Works for me.

~ Johannes


> --paulr