llvm dev - Jan 2019 - Removing LLVM_ALWAYS_INLINE from ADT classes

On Fri, Jan 11, 2019 at 11:18 AM Davide Italiano via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> After yet another round of discussions, the plan is that of trying not
> to slap another attribute on the members, instead going for making
> OPTIMIZED_TLBGEN the default and removing always_inline.
> I'll do some testing locally (for the Ninja and the Xcode build
> generator). Pawel volunteered to take a look at the Visual Studio
> situation.
> I assume this is going to be the final plan we'll deploy unless
> somebody comes with any fundamental objections or there are technical
> hurdles down the road.
>
So, personally I feel strongly that LLVM_OPTIMIZED_TABLEGEN should be off
by default, since the it splits up the ninja build graph. We've also run
into issues with it with VS: https://reviews.llvm.org/D54153  Shelling out
to `cmake --build` as part of a build action doesn't seem to work very well
for many generators.

What actually blocks us from removing these always_inline attributes? When
I read this thread, I didn't get the sense that debug build tablegen
performance was actually important to anyone, but maybe I skimmed too much.

Maybe I am a luddite because I just build Release+Asserts with debug info
enabled and then sprinkle `#pragma clang optimize off` around when I need
to debug a particular source file. This keeps build & test fast enough
except in the corner cases where a debugger is required.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190114/03ab4612/attachment.html>

I agree that shelling out to `cmake --build` as a build step is pretty ugly.

It seems kind of simpler to me to just always build Support and TableGen
with O3 always, even in debug builds. Most people don't debug code in
Support and TableGen, and there could be a disable switch for those that
do. That gives you a fast(er) debug build without building any files twice.
Is that something CMake can do? If so, is this something that was
considered?

(It's not implemented, but in the GN build it's in theory doable to have
a
release toolchain and always use that for tablegen, that way you have the
same effect, but the duplicate build of tablegen is just part of the
regular build. But I figured the "always O3 for lib/Support and
lib/TableGen" approach would be nicer there too. Since I always do Release
builds like Reid and just switch to Debug when I need it, it wasn't a
priority for me to think too much about this though.)

On Mon, Jan 14, 2019 at 4:52 PM Reid Kleckner <rnk at google.com> wrote:
> On Fri, Jan 11, 2019 at 11:18 AM Davide Italiano via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> After yet another round of discussions, the plan is that of trying not
>> to slap another attribute on the members, instead going for making
>> OPTIMIZED_TLBGEN the default and removing always_inline.
>> I'll do some testing locally (for the Ninja and the Xcode build
>> generator). Pawel volunteered to take a look at the Visual Studio
>> situation.
>> I assume this is going to be the final plan we'll deploy unless
>> somebody comes with any fundamental objections or there are technical
>> hurdles down the road.
>>
>
> So, personally I feel strongly that LLVM_OPTIMIZED_TABLEGEN should be off
> by default, since the it splits up the ninja build graph. We've also
run
> into issues with it with VS: https://reviews.llvm.org/D54153  Shelling
> out to `cmake --build` as part of a build action doesn't seem to work
very
> well for many generators.
>
> What actually blocks us from removing these always_inline attributes? When
> I read this thread, I didn't get the sense that debug build tablegen
> performance was actually important to anyone, but maybe I skimmed too much.
>
> Maybe I am a luddite because I just build Release+Asserts with debug info
> enabled and then sprinkle `#pragma clang optimize off` around when I need
> to debug a particular source file. This keeps build & test fast enough
> except in the corner cases where a debugger is required.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190114/77e84d7c/attachment.html>

The *specific* desire is to get methods from Support and ADT to work well
in debug builds. So not sure building those things with O3 helps.

I really don't understand the tension here.

Fundamentally, I think we need to give up on debug builds have super
predictable build-time performance. IMO, that includes tablegen. I'm happy
to have workarounds like building an optimized tablegen binary, but I'm not
sure its worth the complexity that it sounsd like that entails.

(Separately, I really wish Ninja and/or CMake had *good* models for a host
build vs. a target build so that it was easy and reliable to get optimized
host utilities built as well as unoptimized target binaries, but that seems
to be a real problem in their current design and I'm not suggesting we wait
for that to fix any of this.)

On Mon, Jan 14, 2019 at 3:11 PM Nico Weber <thakis at chromium.org> wrote:
> I agree that shelling out to `cmake --build` as a build step is pretty
> ugly.
>
> It seems kind of simpler to me to just always build Support and TableGen
> with O3 always, even in debug builds. Most people don't debug code in
> Support and TableGen, and there could be a disable switch for those that
> do. That gives you a fast(er) debug build without building any files twice.
> Is that something CMake can do? If so, is this something that was
> considered?
>
> (It's not implemented, but in the GN build it's in theory doable to
have a
> release toolchain and always use that for tablegen, that way you have the
> same effect, but the duplicate build of tablegen is just part of the
> regular build. But I figured the "always O3 for lib/Support and
> lib/TableGen" approach would be nicer there too. Since I always do
Release
> builds like Reid and just switch to Debug when I need it, it wasn't a
> priority for me to think too much about this though.)
>
> On Mon, Jan 14, 2019 at 4:52 PM Reid Kleckner <rnk at google.com>
wrote:
>
>> On Fri, Jan 11, 2019 at 11:18 AM Davide Italiano via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> After yet another round of discussions, the plan is that of trying
not
>>> to slap another attribute on the members, instead going for making
>>> OPTIMIZED_TLBGEN the default and removing always_inline.
>>> I'll do some testing locally (for the Ninja and the Xcode build
>>> generator). Pawel volunteered to take a look at the Visual Studio
>>> situation.
>>> I assume this is going to be the final plan we'll deploy unless
>>> somebody comes with any fundamental objections or there are
technical
>>> hurdles down the road.
>>>
>>
>> So, personally I feel strongly that LLVM_OPTIMIZED_TABLEGEN should be
off
>> by default, since the it splits up the ninja build graph. We've
also run
>> into issues with it with VS: https://reviews.llvm.org/D54153  Shelling
>> out to `cmake --build` as part of a build action doesn't seem to
work very
>> well for many generators.
>>
>> What actually blocks us from removing these always_inline attributes?
>> When I read this thread, I didn't get the sense that debug build
tablegen
>> performance was actually important to anyone, but maybe I skimmed too
much.
>>
>> Maybe I am a luddite because I just build Release+Asserts with debug
info
>> enabled and then sprinkle `#pragma clang optimize off` around when I
need
>> to debug a particular source file. This keeps build & test fast
enough
>> except in the corner cases where a debugger is required.
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190114/750d526b/attachment.html>

On Mon, Jan 14, 2019 at 1:52 PM Reid Kleckner <rnk at google.com>
wrote:
>
> On Fri, Jan 11, 2019 at 11:18 AM Davide Italiano via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
>>
>> After yet another round of discussions, the plan is that of trying not
>> to slap another attribute on the members, instead going for making
>> OPTIMIZED_TLBGEN the default and removing always_inline.
>> I'll do some testing locally (for the Ninja and the Xcode build
>> generator). Pawel volunteered to take a look at the Visual Studio
>> situation.
>> I assume this is going to be the final plan we'll deploy unless
>> somebody comes with any fundamental objections or there are technical
>> hurdles down the road.
>
>
> So, personally I feel strongly that LLVM_OPTIMIZED_TABLEGEN should be off
by default, since the it splits up the ninja build graph. We've also run
into issues with it with VS: https://reviews.llvm.org/D54153  Shelling out to
`cmake --build` as part of a build action doesn't seem to work very well for
many generators.
>
> What actually blocks us from removing these always_inline attributes? When
I read this thread, I didn't get the sense that debug build tablegen
performance was actually important to anyone, but maybe I skimmed too much.
>
The only complain I heard was from somebody with a proprietary
backend, but I contacted them again and they said it wasn't a problem
for them anymore.
You're right that nobody had fundamental objections to just drop the
attributes.

--
Davide

On Tue, Jan 15, 2019 at 3:44 PM Davide Italiano <davide at freebsd.org>
wrote:
> On Mon, Jan 14, 2019 at 1:52 PM Reid Kleckner <rnk at google.com>
wrote:
> >
> > On Fri, Jan 11, 2019 at 11:18 AM Davide Italiano via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>
> >> After yet another round of discussions, the plan is that of trying
not
> >> to slap another attribute on the members, instead going for making
> >> OPTIMIZED_TLBGEN the default and removing always_inline.
> >> I'll do some testing locally (for the Ninja and the Xcode
build
> >> generator). Pawel volunteered to take a look at the Visual Studio
> >> situation.
> >> I assume this is going to be the final plan we'll deploy
unless
> >> somebody comes with any fundamental objections or there are
technical
> >> hurdles down the road.
> >
> >
> > So, personally I feel strongly that LLVM_OPTIMIZED_TABLEGEN should be
> off by default, since the it splits up the ninja build graph. We've
also
> run into issues with it with VS: https://reviews.llvm.org/D54153
> Shelling out to `cmake --build` as part of a build action doesn't seem
to
> work very well for many generators.
> >
> > What actually blocks us from removing these always_inline attributes?
> When I read this thread, I didn't get the sense that debug build
tablegen
> performance was actually important to anyone, but maybe I skimmed too much.
> >
>
> The only complain I heard was from somebody with a proprietary
> backend, but I contacted them again and they said it wasn't a problem
> for them anymore.
> You're right that nobody had fundamental objections to just drop the
> attributes.
>
I don't have anything against removing the attributes, but I'm curious
about the "optimized builds" story (I hope it is OK to continue to
hijack
this thread for this?).

I wonder if there is a solution for having the ability to call these inline
functions in an optimized build? Would modules solve this (with the right
debugger support)?

The `__attribute__(used)` could be used to produce "release" build
with
"debug library support" (strawman wording), or better, without source
modification: a compiler flag could be used to automatically add the
"used"
semantic to every inline function (and force emit all the inline function
for all class template instantiation, but that may be too costly).

I wonder if anyone thought about this?

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190115/505e6d17/attachment.html>

On Mon, 14 Jan 2019 at 23:11, Nico Weber via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> I agree that shelling out to `cmake --build` as a build step is pretty
ugly.
>
> It seems kind of simpler to me to just always build Support and TableGen
with O3 always, even in debug builds. Most people don't debug code in
Support and TableGen, and there could be a disable switch for those that do.
That gives you a fast(er) debug build without building any files twice. Is that
something CMake can do? If so, is this something that was considered?
As mentioned elsewhere in the thread, building TableGen with
Debug+Asserts isn't only useful for people who want to debug TableGen
itself. It's useful for anybody modifying .td as many checks on .td
input are only run in an asserts build. If there is a desire to move
to LLVM_OPTIMIZED_TABLEGEN by default I think the correct next step is
to write up a separate RFC on this, detailing the advantages,
disadvantages, and potential paths forward. One of the obvious path
forwards is to put development effort into ensuring that tablegen
doesn't need asserts enabled in order to catch invalid inputs.

Best,

Alex