Page MenuHomePhabricator

[AttrDocs] document always_inline
ClosedPublic

Authored by nickdesaulniers on Oct 3 2019, 11:36 AM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 11:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jyknight added inline comments.
clang/include/clang/Basic/AttrDocs.td
5441–5442

I believe the semantics are stronger than this. It should get inlined unless it cannot be.

lebedev.ri added a subscriber: lebedev.ri.
lebedev.ri added inline comments.
clang/include/clang/Basic/AttrDocs.td
5436–5437

One too many lines

aaron.ballman added inline comments.Oct 3 2019, 11:44 AM
clang/include/clang/Basic/AttrDocs.td
5442

It may make sense to link back to MSDN and GCC if we're honoring their semantics. We seem to do this for MSDN regularly, but a bit less common for the GCC docs.

5444

Can you add the various spellings to this heading?

nickdesaulniers edited the summary of this revision. (Show Details)Oct 3 2019, 12:56 PM
jdoerfert added inline comments.
clang/include/clang/Basic/AttrDocs.td
5443

It is more than that. This would imply that with optimizations enabled there is no effect. I would mention that the inline heuristic is disabled and inlining is always attempted, w/ or w/o optimizations.

nickdesaulniers marked 3 inline comments as done.
  • add links
  • remove extra whitespace
  • rewording, split onto separate lines to ease code review
  • add spellings to heading
nickdesaulniers marked an inline comment as done.Oct 3 2019, 3:27 PM
nickdesaulniers added inline comments.
clang/include/clang/Basic/AttrDocs.td
5441–5442

Seemingly not for indirect function calls: https://godbolt.org/z/BoND-X

Just linking relevant bug for the record: https://bugs.llvm.org/show_bug.cgi?id=43517

Also, I'm fairly certain __forceinline and always_inline, confusingly enough differ in semantics, with __forceinline only being a stronger hint on MSVC.

joerg added a subscriber: joerg.Oct 5 2019, 4:17 PM

I wonder if we should actually enumerate evil here, i.e. give the situations in which inlining actually fails. As mentioned on IRC, I wonder if we shouldn't aim for the stronger semantics and at least warn by default of any situation that prevents always_inline from doing its job.

Also, I'm fairly certain __forceinline and always_inline, confusingly enough differ in semantics, with __forceinline only being a stronger hint on MSVC.

Does clang handle __forceinline vs always_inline differently, today? If not, then sounds like we may need to split these in two.

I wonder if we should actually enumerate evil here, i.e. give the situations in which inlining actually fails.

Which is likely to change over time. I worry that enumerating such cases is compiler version specific, and might lead to developers depending/[ab]using that behavior?

As mentioned on IRC, I wonder if we shouldn't aim for the stronger semantics

As long as we error when we fail to inline, I think that matches GCC's behavior. There's likely differences in what we can inline or not.

and at least warn by default of any situation that prevents always_inline from doing its job.

Might be hard to recognize all such cases in the frontend? GCC does warn via -Wattributes when the attribute is applied to a non-inline function.

I wonder if we should actually enumerate evil here, i.e. give the situations in which inlining actually fails.

Which is likely to change over time. I worry that enumerating such cases is compiler version specific, and might lead to developers depending/[ab]using that behavior?

I'm jumping in part-way here, but FWIW I'd be concerned that the lack of explicit rules is actually going to make people depend on the whims of the optimizer/inliner - if we have a backend warning that only fires when inlining doesn't occur. So narrowing the set of cases we accept in the frontend seems like it reduces the risk of abuse by compiler users - leaving more freedom for the compiler optimizations to change without breaking existing code.

lebedev.ri requested changes to this revision.Oct 16 2019, 10:54 AM

I think partial (but not wrong!) docs is better than no docs whatsoever, so i'd be inclined to proceed with this.
I, too, not really convinced that actual explicit rules should be spelled out.
Some nits, LG otherwise to me.

clang/include/clang/Basic/AttrDocs.td
5440

s/Inline/Inlining/

5443

This comment wasn't addressed.

This revision now requires changes to proceed.Oct 16 2019, 10:54 AM
nickdesaulniers marked an inline comment as done.
  • rebased, s/Inline/Inlining/

Sorry, I should not have waiting this long to update this patch...

clang/include/clang/Basic/AttrDocs.td
5443

I don't follow. I'm not sure if this comment has bit rot? I think it's critical to mention that always_inline does not mean always, as inline substitution MAY fail.

LGTM

clang/include/clang/Basic/AttrDocs.td
5443

Original text:

Hint that inline substitution should be attempted when optimizations are

disabled. Does not guarantee that inline substitution actually occurs.

The new text is better.

ojeda added inline comments.Dec 6 2020, 7:01 AM
clang/include/clang/Basic/AttrDocs.td
5665

MSDN is gone now, Microsoft Docs is the new one.

LGTM aside from the minor wording nit from @ojeda.

s/MSDN/Microsoft Docs/

nickdesaulniers marked 4 inline comments as done.Dec 7 2020, 11:32 AM

@lebedev.ri would you mind re-reviewing when you have a chance?

ojeda accepted this revision.Dec 7 2020, 12:34 PM
lebedev.ri resigned from this revision.Dec 16 2020, 11:37 PM

Seems fine to me.

This revision is now accepted and ready to land.Dec 16 2020, 11:37 PM
This revision was automatically updated to reflect the committed changes.