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

[Chris Lattner]

On Aug 12, 2009, at 12:34 PM, Gabor Greif wrote:
>> > 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.

Gabor and I briefly discussed this on irc, the takeaways:

1. we shouldn't parameterize based on configure stuff, the assert in  
question should just unconditionally go away.
2. this allows the recent configure/cmake stuff to go away
3. the actual data structure change will be deferred until after 2.6  
branches, I'm nervous about potential fallout from it.

Thank you again for working on this Gabor, it seems like a big memory  
win!

-Chris

>
> 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/ec84e49b/attachment.html>