[llvm-commits] Compact Sentinels [RE: r78628 - /llvm/trunk/autoconf/configure.ac]

[Gabor Greif]

> On Aug 11, 2009, at 2:22 PM, Gabor Greif wrote:
> >>> URL:http://llvm.org/viewvc/llvm-project?rev=78628&view=rev
> >>> Log:
> >>> Lay the groundwork for my upcoming ilist sentinel shrinking patch
> >>> by defining a LLVM_COMPACT_SENTINELS symbol to 0 or 1 in config.h.
> >>
> >> Gabor, it is usually a bad idea to make major ABI changes like this
> >> based on build settings like ENABLE_OPTIMIZED and  
> DISABLE_ASSERTIONS.
> >> Why do you need this?
> >
> > Hi Chris,
> >
> > the reason is that I originally had a patch in the tree where
> > NDEBUG caused the sentinels to only have a back pointer, but no
> > forward pointer.
> > Because NDEBUG disabled assertions, no one looked at the forward
> > pointer anyway.
>
> But why do you want different behavior based on whether assertions are
> enabled or not?  Again, that's another indicator that major and
> invasive abi changes based on assertions being enable is a problem,
> working around it doesn't make the problem go away. :)

Hi Chris,

I think the problem goes away.

I want to decouple from NDEBUG. This is why I have introduced a  
config.h symbol
named LLVM_COMPACT_SENTINELS.

It gets now persistently set up at configure time given
following rules:

--enable-release && --disable-asserts => LLVM_COMPACT_SENTINELS = 1

for CMAKE Release mode (already normally implies  
LLVM_ENABLE_ASSERTIONS = 0)
implies LLVM_COMPACT_SENTINELS = 1

Rationale.

There are some asserts in ilist.h that check that no operator++
is performed on the end iterator etc. These asserts only make
sense when the sentinel has a "Next" field allocated.

When the user wants asserts IMHO, she should get all of them.
This means --enable-asserts => LLVM_COMPACT_SENTINELS = 1.


>
> > Now, this turned out to be a suboptimal idea, because llvm-gcc  
> (e.g.)
> > sets NDEBUG
> > in some rather strange ways, so the clients sometimes were  
> configured
> > differently,
> > causing major disagreements in the size and field offset of
> > llvm::Function, for example.
>
> Right, this is an example of why we don't want abi changes :)

Right, this is why now there is a permanent symbol in Config/config.h
which makes the behaviour predictable. Any library built with this
symbol unaltered will be ABI compatible across all clients. N.B.: This
is pretty much like the (made up symbol) IS_BIG_ENDIAN.

>
> > Now the feature of lean sentinels is an interesting one, since it
> > saves 2 pointers per
> > llvm::Function and one pointer per llvm::BasicBlock, without any
> > additional overhead.
>
> Wow, that would be great.  Can you describe the approach?  Should this
> wait until after 2.6 branches?  I'm somewhat nervous about invasive
> changes like this, and we've had issues in the past with the previous
> ilist shrinking patches.

I know you would like this ;-)

Now, already almost all (the frequently used ones) ilist<T> objects  
have their
sentinel aggregated (I call them ghostly sentinels, because they live  
inside the
ilist and superpose with the object that contains the ilist.

The sentinel currently also has 2 pointers, like all ilist_nodes.

Here is a bit of ASCII art to show it:

              +-------------------+--+
              v                   |  |
              <-ghostly!->        |  |
    Sentinel: |Elem:  bf |        |  |
*-----------------------+----*   |  |
|Obj:      | ilist: Bbf | ...|   |  |
*-------------------||--+----*   |  |
                     ||           |  |  Legend:
     +---------------+|           |  |   B: begin()
     |                + ----+     |  |   b: back
     v       +--------------|-----+  |   f: forward
     *-------|--*           |        |
     |Elem:  bf |           |        |
     *--------|-*           |        |
     ^        +-------------+        |
     |                      v        |
     |                      *--------|-*
     |                      |Elem:  bf |
     |                      *-------|--*
     +------------------------------+

As you can see, the forward pointer of the sentinel is never
accessed (save the asserts verifying the legal operator++ invocations),
because it is forbidden to dereference the end() iterator.

So my old patch eliminated the forward pointer from the sentinel, thus
shrinking ilist by a pointer and thus also Obj.

I do not think that this is a risky issue, and there is still a bunch  
of time
till 2.6 branches. The patch proper is just about 10 lines of code  
and can
be backed out with ease if something goes wrong (which I do not expect).

>
> > This is how I came up with LLVM_COMPACT_SENTINELS in configure. It
> > defaults to 1 if
> > in Release-Asserts mode. Now if clients include the same config.h  
> and
> > link against the
> > Release-Asserts libraries, there will be no trouble. I guess this is
> > the primary use case.
>
> How about just making it a autoconf switch decoupled from either
> existing setting?  Why exactly don't you want this for non-assert  
> mode?

If I make an autoconf setting for this, then nobody will use it,  
which entails
lost benefits.

I think the automatism of "Release mode without asserts" is exactly  
when I
want the benefits of this optimization. But I am not religious about  
this.

Cheers,

	Gabor

>
> -Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090812/0d8f61a0/attachment.html>