llvm dev - Jan 2019 - Removing LLVM_ALWAYS_INLINE from ADT classes

On Thu, Jan 10, 2019 at 1:20 PM Davide Italiano <davide at freebsd.org>
wrote:
>
> On Wed, Jan 9, 2019 at 2:18 PM Davide Italiano <davide at
freebsd.org> wrote:
> >
> > On Wed, Jan 9, 2019 at 9:38 AM Mehdi AMINI <joker.eph at
gmail.com> wrote:
> > >
> > >
> > >
> > > On Fri, Jan 4, 2019 at 3:15 PM Davide Italiano via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> > >>
> > >> Hi,
> > >> I would like to propose, based on a previous discussion on
llvm-dev,
> > >> the following change.
> > >> https://reviews.llvm.org/D56337
> > >>
> > >> The main motivation for annotating member functions of ADT
clases with
> > >> LLVM_ALWAYS_INLINE  was that of speeding up `check-llvm` at
`-O0`.
> > >> Turns out this significantly degrades the debuggability of
fundamental
> > >> classes in llvm itself, e.g. StringRef or SmallVector.
> > >
> > >
> > > It seems that the issue is not a problem with the inlining these
functions but rather that they aren't emitted when inlined and so can't
be called from the debugger.
> > >
> > > An alternative that preserves the inlining for the performance
aspect *and* still emit them in debug build so that they are available in debug
builds would be another macro that would expand to something like
__attribute__((used)).
> > >
> >
>
> Your suggestion works just fine. I'll update the patch.
>
> * thread #1, queue = 'com.apple.main-thread', stop reason = step
over
>     frame #0: 0x000000010000a110 lldb`main(argc=1,
> argv=0x00007ffeefbff9c8) at Driver.cpp:872
>    869
>    870  // Print stack trace on crash.
>    871  llvm::StringRef ToolName = llvm::sys::path::filename(argv[0]);
> -> 872  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
>    873  llvm::PrettyStackTraceProgram X(argc, argv);
>    874
>    875  // Parse arguments.
>
> (lldb) p ToolName.size()
> (size_t) $2 = 4
> (lldb) p ToolName.consume_front(llvm::StringRef("l"))
> (bool) $4 = true
> (lldb) p ToolName.consume_front(llvm::StringRef("a"))
> (bool) $5 = false
> (lldb) p ToolName.slice(1,2)
> (llvm::StringRef) $3 = (Data = "ldb", Length = 1)
>
> Thank you!
New review online: https://reviews.llvm.org/D56567

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare

On Thu, Jan 10, 2019 at 3:57 PM Davide Italiano <davide at freebsd.org>
wrote:
>
> On Thu, Jan 10, 2019 at 1:20 PM Davide Italiano <davide at
freebsd.org> wrote:
> >
> > On Wed, Jan 9, 2019 at 2:18 PM Davide Italiano <davide at
freebsd.org> wrote:
> > >
> > > On Wed, Jan 9, 2019 at 9:38 AM Mehdi AMINI <joker.eph at
gmail.com> wrote:
> > > >
> > > >
> > > >
> > > > On Fri, Jan 4, 2019 at 3:15 PM Davide Italiano via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> > > >>
> > > >> Hi,
> > > >> I would like to propose, based on a previous discussion
on llvm-dev,
> > > >> the following change.
> > > >> https://reviews.llvm.org/D56337
> > > >>
> > > >> The main motivation for annotating member functions of
ADT clases with
> > > >> LLVM_ALWAYS_INLINE  was that of speeding up `check-llvm`
at `-O0`.
> > > >> Turns out this significantly degrades the debuggability
of fundamental
> > > >> classes in llvm itself, e.g. StringRef or SmallVector.
> > > >
> > > >
> > > > It seems that the issue is not a problem with the inlining
these functions but rather that they aren't emitted when inlined and so
can't be called from the debugger.
> > > >
> > > > An alternative that preserves the inlining for the
performance aspect *and* still emit them in debug build so that they are
available in debug builds would be another macro that would expand to something
like __attribute__((used)).
> > > >
> > >
> >
> > Your suggestion works just fine. I'll update the patch.
> >
> > * thread #1, queue = 'com.apple.main-thread', stop reason =
step over
> >     frame #0: 0x000000010000a110 lldb`main(argc=1,
> > argv=0x00007ffeefbff9c8) at Driver.cpp:872
> >    869
> >    870  // Print stack trace on crash.
> >    871  llvm::StringRef ToolName = llvm::sys::path::filename(argv[0]);
> > -> 872  llvm::sys::PrintStackTraceOnErrorSignal(ToolName);
> >    873  llvm::PrettyStackTraceProgram X(argc, argv);
> >    874
> >    875  // Parse arguments.
> >
> > (lldb) p ToolName.size()
> > (size_t) $2 = 4
> > (lldb) p ToolName.consume_front(llvm::StringRef("l"))
> > (bool) $4 = true
> > (lldb) p ToolName.consume_front(llvm::StringRef("a"))
> > (bool) $5 = false
> > (lldb) p ToolName.slice(1,2)
> > (llvm::StringRef) $3 = (Data = "ldb", Length = 1)
> >
> > Thank you!
>
> New review online: https://reviews.llvm.org/D56567
>
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.

Thanks,

--
Davide

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>