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

[David Blaikie via llvm-dev]

On Sun, Apr 25, 2021 at 2:38 PM Fangrui Song <maskray at google.com> wrote:
>
> On 2021-04-22, Johannes Doerfert via llvm-dev wrote:
> >
> >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
>
> I see that the clang attribute 'optnone' patch rC205255 (in 2014) added
> both the IR 'optnone' and 'noinline' attributes.
>
> If the clang attribute 'optnone' (for debugging purposes) is to be renamed,
> I humbly suggest we may consider implementing __attribute__((optimize("O0")))
> (limited to "O0" only; other values are not accepted).
>
> https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html says
> "The optimize attribute should be used for debugging purposes only. It
> is not suitable in production code." which matches our debugging only
> purposes.
>
> -O0 code already emits 'optnone' and 'noinline' for non-alwaysinline
> functions, so we may not need a new attribute.

I don't have any plans to rename or add clang attributes - though that
one could be added as an alias for whatever the optnone clang
attribute does.