This is an archive of the discontinued LLVM Phabricator instance.

Differential D56337

[ADT] Remove attribute LLVM_ALWAYS_INLINE from several classes.
ClosedPublic

Authored by davide on Jan 4 2019, 3:09 PM.
Tokens
"Like" token, awarded by jkorous.

Details

Reviewers
chandlerc
JDevlieghere
friss
aprantl
Commits
rG078fb93c3d8c: [Docs] Add a note clarifying how to get good test performances.
rL351885: [Docs] Add a note clarifying how to get good test performances.
Summary

The original motivation for this was trying to get faster turnaround
when running check-llvm at -O0. The impact on debuggability seems
to outweight the benefits, as it makes StringRef and other ADT
classes fairly undebuggable.

Add a section in the developer's documentation explaining that
optimized builds are needed for decent test performances instead.

Based on a discussion on llvm-dev, pending more general consensus.

Diff Detail

Event Timeline

davide created this revision.Jan 4 2019, 3:09 PM
Herald added a subscriber: dexonsmith. · View Herald TranscriptJan 4 2019, 3:09 PM
Harbormaster completed remote builds in B26412: Diff 180331.Jan 4 2019, 3:11 PM
chandlerc accepted this revision.Jan 4 2019, 3:40 PM

LGTM.

There are a bunch of others that need to go, but those can be follow-ups.

This revision is now accepted and ready to land.Jan 4 2019, 3:40 PM
JDevlieghere added inline comments.Jan 4 2019, 3:48 PM
llvm/docs/TestingGuide.rst
110

Should we rephrase this in a way that makes it clear that you don't always have to set the CMake flag, e.g. in release? Maybe something like `build ... in release or at least with -O2"?

chandlerc added inline comments.Jan 4 2019, 3:58 PM
llvm/docs/TestingGuide.rst
110

That's a good point, we should instead suggest enabling one of teh release build modes rather than hard coding flags here.

Maybe clarify that those are independent of LLVM_ENABLE_ASSERTIONS.

aprantl accepted this revision.Jan 7 2019, 9:27 AM
aprantl added inline comments.
llvm/docs/TestingGuide.rst
110

nit: I think it should be performance (singular)?

aprantl added inline comments.Jan 7 2019, 9:29 AM
llvm/docs/TestingGuide.rst
115

Do people manually tune their -O flags? Wouldn't it be more straightforward to just recommend to build/run the tests in -DCMAKE_BUILD_TYPE=Release or RelWithDebInfo mode?

jkorous awarded a token.Jan 8 2019, 1:57 AM
davide updated this revision to Diff 182976.Jan 22 2019, 1:48 PM

Update the wording.

JDevlieghere accepted this revision.Jan 22 2019, 1:49 PM

LGTM

Closed by commit rL351885: [Docs] Add a note clarifying how to get good test performances. (authored by davide). · Explain WhyJan 22 2019, 1:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
docs/
7 lines

Diff 182976

llvm/docs/TestingGuide.rst

Context not available.
% make check-llvm % make check-llvm
In order to get reasonable testing performance, build LLVM and subprojects
JDevlieghereUnsubmitted
Not Done
ReplyInline Actions

Should we rephrase this in a way that makes it clear that you don't always have to set the CMake flag, e.g. in release? Maybe something like `build ... in release or at least with -O2"?

JDevlieghere: Should we rephrase this in a way that makes it clear that you don't always have to set the…
chandlercUnsubmitted
Not Done
ReplyInline Actions

That's a good point, we should instead suggest enabling one of teh release build modes rather than hard coding flags here.

Maybe clarify that those are independent of LLVM_ENABLE_ASSERTIONS.

chandlerc: That's a good point, we should instead suggest enabling one of teh release build modes rather…
aprantlUnsubmitted
Not Done
ReplyInline Actions

nit: I think it should be performance (singular)?

aprantl: nit: I think it should be `performance` (singular)?
in release mode, i.e.
.. code-block:: bash
% cmake -DCMAKE_BUILD_TYPE="Release"
aprantlUnsubmitted
Not Done
ReplyInline Actions

Do people manually tune their -O flags? Wouldn't it be more straightforward to just recommend to build/run the tests in -DCMAKE_BUILD_TYPE=Release or RelWithDebInfo mode?

aprantl: Do people manually tune their -O flags? Wouldn't it be more straightforward to just recommend…
If you have `Clang <http://clang.llvm.org/>`_ checked out and built, you If you have `Clang <http://clang.llvm.org/>`_ checked out and built, you
can run the LLVM and Clang tests simultaneously using: can run the LLVM and Clang tests simultaneously using:
Context not available.