llvm dev - Jan 2019 - Removing LLVM_ALWAYS_INLINE from ADT classes

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 Tue, Jan 15, 2019 at 4:11 PM Mehdi AMINI <joker.eph at gmail.com>
wrote:
>
>
> 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).
>
One of the patches that Davide tried out did essentially this. But it did
add even more attribute weight to all these APIs.

Doing it without source modification would be great, and modules (or
generally modules-based debug info instead of DWARF based debug info) makes
that somewhat easier. Lots of other things that could be done here.

But I think a lot of this is more long-term. I'd really like us to do
something in the short term and so far, this patch seems like the cleanest
short-term solution.

I think we can continue to explore whether and to what extent it might be
worth shifting the defaults around tablegen after this goes in? While I
agree that the unoptimized tablegen is perhaps one of the most striking
ways in which a debug build w/o hacks like always-inline is extremely slow,
it isn't clear we have a clean or short-term solution in that space.

So I would suggest we continue with this patch, and then keep exploring
both how we could make tablegen less painful for typical developers (maybe
some combination of better nested build support and better error management
in tablegen), and better optimized debug experiences so that at least some
of the time people can just develop on an optimized build with asserts and
debug info enabled and get the basics working fine in their debugger.

My two cents.
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190116/40b0ea35/attachment-0001.html>

On Wed, Jan 16, 2019 at 2:03 AM Chandler Carruth <chandlerc at gmail.com>
wrote:
>
> On Tue, Jan 15, 2019 at 4:11 PM Mehdi AMINI <joker.eph at gmail.com>
wrote:
>>
>>
>>
>> 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).
>
>
> One of the patches that Davide tried out did essentially this. But it did
add even more attribute weight to all these APIs.
>
> Doing it without source modification would be great, and modules (or
generally modules-based debug info instead of DWARF based debug info) makes that
somewhat easier. Lots of other things that could be done here.
>
> But I think a lot of this is more long-term. I'd really like us to do
something in the short term and so far, this patch seems like the cleanest
short-term solution.
>
> I think we can continue to explore whether and to what extent it might be
worth shifting the defaults around tablegen after this goes in? While I agree
that the unoptimized tablegen is perhaps one of the most striking ways in which
a debug build w/o hacks like always-inline is extremely slow, it isn't clear
we have a clean or short-term solution in that space.
>
> So I would suggest we continue with this patch, and then keep exploring
both how we could make tablegen less painful for typical developers (maybe some
combination of better nested build support and better error management in
tablegen), and better optimized debug experiences so that at least some of the
time people can just develop on an optimized build with asserts and debug info
enabled and get the basics working fine in their debugger.
>
Looks like this is the path we're following. I'll land my original
patches later this week (now that the branch was cut).
If you want to add something to the conversation, feel free, but
please let's try to keep this focused.
We can handle the tblgen situation in a separate thread.

--
Davide