Removing LLVM_ALWAYS_INLINE from ADT classes

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.

After discussing we agreed that it's reasonable to drop
LLVM_ALWYAS_INLINE from ADT classes member functions and add a note
in the developer's manual.

If you have any feedbacks or concerns, please speak up.
If nothing major arises, I'm going to commit this patch in a week (or such).

Thanks for your time,

This makes sense to me.

One concern is that this in itself will slow down the build, since tablegen will get even slower. Ideally, there would be some (perhaps default?) configuration where we build the tablegen binaries with optimizations on and then use them in the build, as if we were cross-compiling.

+1, I think that’s a great change.

One minor correction, this isn’t about ADT.

There are numerous places where we’ve tried to mitigate -O0 test runtime with these attributes that will end up cleaned up based on this direction, not just in ADT. =D

One minor correction, this isn't about ADT.

There are *numerous* places where we've tried to mitigate -O0 test runtime with these attributes that will end up cleaned up based on this direction, not just in ADT. =D

Sure, I'll do these next.
FWIW, I started from the ADT ones because they've been the one disturbing me.
There are other few instances, grouped in three files:
1) Chrono.h
2) SelectionDAGISel.cpp
3) User.h

Admittedly the only other one I hit was 3). I don't work on backends a
lot so maybe 2) is really annoying but I just never experienced it.

This makes sense to me.

One concern is that this in itself will slow down the build, since tablegen will get even slower. Ideally, there would be some (perhaps default?) configuration where we build the tablegen binaries with optimizations on and then use them in the build, as if we were cross-compiling.

Maybe something simple like -DLLVM_OPTIMIZE_TABLEGEN=ON (enabled by default for Debug builds). You would need to set it to off only if you want to debug tablegen.

This makes sense to me.

One concern is that this in itself will slow down the build, since tablegen will get even slower. Ideally, there would be some (perhaps default?) configuration where we build the tablegen binaries with optimizations on and then use them in the build, as if we were cross-compiling.

Maybe something simple like -DLLVM_OPTIMIZE_TABLEGEN=ON (enabled by default for Debug builds). You would need to set it to off only if you want to debug tablegen.

+1 on this. If you don’t hack on tablegen this is a big win.

I proposed this a while back, but it think there were some problems with it, I don’t remember what exactly but maybe related to bootstrapping / 2-stages builds?

  • Chris, maybe he remembers something?

IIRC the issues with defaulting to –DLLVM_OPTIMIZE_TABLEGEN=ON came up in two places:

First, if it’s not a Debug build, this actually slows down the build. The defaulting has to be for Debug config only. This is arguably bad UI, because it makes the default modal, but there’s a clear benefit in build time so I think we can live with that.

Second, if it’s a multi-config builder (e.g. MSBuild) then it gets a little weird, I don’t remember exactly how.

–paulr

LLVM_OPTIMIZED_TABLEGEN=ON is a handy way of speeding up build times,
but it really shouldn't be recommended for anyone who is modifying .td
files. You run the risk of making .td modifications that trigger
asserts for those not using LLVM_OPTIMIZED_TABLEGEN.

Best,

Alex

From: llvm-dev [mailto:llvm-dev-bounces@lists.llvm.org] On Behalf Of Alex
Bradbury via llvm-dev
Sent: Monday, January 07, 2019 11:33 AM
To: Mehdi AMINI
Cc: LLVM Dev
Subject: Re: [llvm-dev] Removing LLVM_ALWAYS_INLINE from ADT classes

>
>
>
>>
>>>
>>> This makes sense to me.
>>>
>>> One concern is that this in itself will slow down the build, since
tablegen will get even slower. Ideally, there would be some (perhaps
default?) configuration where we build the tablegen binaries with
optimizations on and then use them in the build, as if we were cross-
compiling.
>>
>>
>> Maybe something simple like -DLLVM_OPTIMIZE_TABLEGEN=ON (enabled by
default for Debug builds). You would need to set it to off only if you
want to debug tablegen.
>
>
> I proposed this a while back, but it think there were some problems with
it, I don't remember what exactly but maybe related to bootstrapping / 2-
stages builds?
>
> + Chris, maybe he remembers something?

LLVM_OPTIMIZED_TABLEGEN=ON is a handy way of speeding up build times,
but it really shouldn't be recommended for anyone who is modifying .td
files. You run the risk of making .td modifications that trigger
asserts for those not using LLVM_OPTIMIZED_TABLEGEN.

Can we make LLVM_OPTIMIZED_TABLEGEN=ON build it as Release+Asserts?
--paulr

There is (use to be) some very expensive assertions in llvm-tblgen.

I believe the issues about Debug config and Visual Studio can be solved by using generator expressions

$<$<CONFIG:Debug>:-O2>

or alternatively CMAKE_CXX_CLAGS_DEBUG.

At first we don’t have to modify the assertions build option so if Debug config builds with assertions tablegen will be build with as well.

Also we can start with keeping LLVM_OPTIMIZE_TABLEGEN off by default.

If that sounds good, I can come with a prototype later.

// P.

Historically there was a request to make LLVM_OPTIMIZED_TABLEGEN On by default for debug builds, which I discouraged. At the time it was suggested that workflow was fairly new and untested in a lot of build configurations. Today I believe that situation is slightly better.

I believe there are still issues with it not working correctly in the Xcode generator, but I know people have used it successfully with Visual Studio.

I think that enabling it by default in Debug builds is probably reasonable for most generators, but anyone looking to make that change should expect some bumps in the road.

-Chris

IIRC the issues with defaulting to –DLLVM_OPTIMIZE_TABLEGEN=ON came up in two places:

First, if it’s not a Debug build, this actually slows down the build. The defaulting has to be for Debug config only. This is arguably bad UI, because it makes the default modal, but there’s a clear benefit in build time so I think we can live with that.

Second, if it’s a multi-config builder (e.g. MSBuild) then it gets a little weird, I don’t remember exactly how.

–paulr

Out of curiosity, why does it slow down a non-debug build?

The way it is implemented is to create another build directory inside the build directory (called “native” IIRC) and call cmake into it and build llvm-tblgen there. However that means that a library like Support is built here before being rebuilt in the main build directory.

IIRC the issues with defaulting to –DLLVM_OPTIMIZE_TABLEGEN=ON came up in two places:

First, if it’s not a Debug build, this actually slows down the build. The defaulting has to be for Debug config only. This is arguably bad UI, because it makes the default modal, but there’s a clear benefit in build time so I think we can live with that.

Second, if it’s a multi-config builder (e.g. MSBuild) then it gets a little weird, I don’t remember exactly how.

–paulr

Out of curiosity, why does it slow down a non-debug build?

The way it is implemented is to create another build directory inside the build directory (called “native” IIRC) and call cmake into it and build llvm-tblgen there. However that means that a library like Support is built here before being rebuilt in the main build directory.

I see, thanks!

Although it's super handy to have it available, IMHO the risks of
writing, submitting, and ultimately committing incorrect .td
modifications mean that making LLVM_OPTIMIZED_TABLEGEN the default
would be unwise. It might be more feasible if there were a 'check-td'
target that put all the .td specified in the CMakeLists for each
configured target through a debug+asserts or release+asserts tblgen.

In fact maybe there should be a warning in the current CMake docs to
indicate of using a no-asserts tblgen binary while hacking on LLVM.

Best,

Alex

Historically there was a request to make LLVM_OPTIMIZED_TABLEGEN On by default for debug builds, which I discouraged. At the time it was suggested that workflow was fairly new and untested in a lot of build configurations. Today I believe that situation is slightly better.

I believe there are still issues with it not working correctly in the Xcode generator, but I know people have used it successfully with Visual Studio.

I think that enabling it by default in Debug builds is probably reasonable for most generators, but anyone looking to make that change should expect some bumps in the road.

Although it’s super handy to have it available, IMHO the risks of
writing, submitting, and ultimately committing incorrect .td
modifications mean that making LLVM_OPTIMIZED_TABLEGEN the default
would be unwise.

But modifying a .td is not necessarily part of the main routine of developing on LLVM.

It might be more feasible if there were a ‘check-td’
target that put all the .td specified in the CMakeLists for each
configured target through a debug+asserts or release+asserts tblgen.

In fact maybe there should be a warning in the current CMake docs to
indicate of using a no-asserts tblgen binary while hacking on LLVM.

How do you detect what is « hacking on LLVM » when running Cmake? Why do you single out tblgen assertions?

I was using an « external » llvm-tblgen most of the time I was hacking on LLVM to avoid constantly rebuilding it.

More importantly: assertions should catch internal issues with tblgen itself. Issues with invalid .td should not be implemented through assertions IMO but always-on checks.

I can see both sides of this argument, but fundamentally I agree with Mehdi that we shouldn’t treat table gen assertions differently from LLVM. We have no mechanism for enforcing that people build and test with assertions enabled before committing code, we also have no mechanism for enforcing verify-machine-instrs or a large number of other configurations that catch bugs. In the current development model we just expect that the bots will catch it all.

We could make the default tablegen build configuration Release+Asserts, and add an additional option to disable the asserts, but I’m not entirely convinced that is the right approach given how costly some of tablegen’s asserts are.

-Chris