<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Aug 12, 2009, at 12:34 PM, Gabor Greif wrote:</div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><blockquote type="cite"><div>> the reason is that I originally had a patch in the tree where</div><div>> NDEBUG caused the sentinels to only have a back pointer, but no</div><div>> forward pointer.</div><div>> Because NDEBUG disabled assertions, no one looked at the forward</div><div>> pointer anyway.</div><div><br></div><div>But why do you want different behavior based on whether assertions are </div><div>enabled or not? Again, that's another indicator that major and </div><div>invasive abi changes based on assertions being enable is a problem, </div><div>working around it doesn't make the problem go away. :)</div></blockquote><div><br></div>Hi Chris,</div><div><br></div><div>I think the problem goes away.<br><div><br></div><div>I want to decouple from NDEBUG. This is why I have introduced a config.h symbol</div><div>named LLVM_COMPACT_SENTINELS.</div></div></div></blockquote><div><br></div><div>Gabor and I briefly discussed this on irc, the takeaways:</div><div><br></div><div>1. we shouldn't parameterize based on configure stuff, the assert in question should just unconditionally go away.</div><div>2. this allows the recent configure/cmake stuff to go away</div><div>3. the actual data structure change will be deferred until after 2.6 branches, I'm nervous about potential fallout from it.</div><div><br></div><div>Thank you again for working on this Gabor, it seems like a big memory win!</div><div><br></div><div>-Chris</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><div>It gets now persistently set up at configure time given</div><div>following rules:</div><div><br></div><div>--enable-release && --disable-asserts => LLVM_COMPACT_SENTINELS = 1</div><div><br></div><div>for CMAKE Release mode (already normally implies LLVM_ENABLE_ASSERTIONS = 0)</div><div>implies LLVM_COMPACT_SENTINELS = 1</div><div><br></div><div>Rationale.</div><div><br></div><div>There are some asserts in ilist.h that check that no operator++</div><div>is performed on the end iterator etc. These asserts only make</div><div>sense when the sentinel has a "Next" field allocated.</div><div><br></div><div>When the user wants asserts IMHO, she should get all of them.</div><div>This means --enable-asserts => LLVM_COMPACT_SENTINELS = 1.</div><div><br></div><div><br></div><blockquote type="cite"><div><br></div><div>> Now, this turned out to be a suboptimal idea, because llvm-gcc (e.g.)</div><div>> sets NDEBUG</div><div>> in some rather strange ways, so the clients sometimes were configured</div><div>> differently,</div><div>> causing major disagreements in the size and field offset of</div><div>> llvm::Function, for example.</div><div><br></div><div>Right, this is an example of why we don't want abi changes :)</div></blockquote><div><br></div><div>Right, this is why now there is a permanent symbol in Config/config.h</div><div>which makes the behaviour predictable. Any library built with this</div><div>symbol unaltered will be ABI compatible across all clients. N.B.: This</div><div>is pretty much like the (made up symbol) IS_BIG_ENDIAN.</div><br><blockquote type="cite"><div><br></div><div>> Now the feature of lean sentinels is an interesting one, since it</div><div>> saves 2 pointers per</div><div>> llvm::Function and one pointer per llvm::BasicBlock, without any</div><div>> additional overhead.</div><div><br></div><div>Wow, that would be great. Can you describe the approach? Should this </div><div>wait until after 2.6 branches? I'm somewhat nervous about invasive </div><div>changes like this, and we've had issues in the past with the previous </div><div>ilist shrinking patches.</div></blockquote><div><br></div>I know you would like this ;-)</div><div><br></div><div>Now, already almost all (the frequently used ones) ilist<T> objects have their</div><div>sentinel aggregated (I call them ghostly sentinels, because they live inside the</div><div>ilist and superpose with the object that contains the ilist.</div><div><br></div><div>The sentinel currently also has 2 pointers, like all ilist_nodes.</div><div><br></div><div>Here is a bit of ASCII art to show it:</div><div><br></div><div><font class="Apple-style-span" face="Courier"> +-------------------+--+</font></div><div><font class="Apple-style-span" face="Courier"> v | |</font></div><div><span class="Apple-style-span" style="font-family: Courier; "> <-ghostly!-> | |</span></div><div><span class="Apple-style-span" style="font-family: Courier; "> Sentinel: |Elem: bf | | |</span></div><div><font class="Apple-style-span" face="Courier">*-----------------------+----* | |</font></div><div><font class="Apple-style-span" face="Courier">|Obj: | ilist: Bbf | ...| | |</font></div><div><font class="Apple-style-span" face="Courier">*-------------------||--+----* | |</font></div><div><font class="Apple-style-span" face="Courier"> || | | Legend:</font></div><div><font class="Apple-style-span" face="Courier"> +---------------+| | | B: begin()</font></div><div><span class="Apple-style-span" style="font-family: Courier; "> | + ----+ | | b: back</span></div><div><span class="Apple-style-span" style="font-family: Courier; "> v +--------------|-----+ | f: forward</span></div><div><font class="Apple-style-span" face="Courier"> *-------|--* | |</font></div><div><font class="Apple-style-span" face="Courier"> |Elem: bf | | |</font></div><div><font class="Apple-style-span" face="Courier"> *--------|-* | |</font></div><div><font class="Apple-style-span" face="Courier"> ^ +-------------+ |</font></div><div><font class="Apple-style-span" face="Courier"> | v |</font></div><div><font class="Apple-style-span" face="Courier"> | *--------|-*</font></div><div><div><font class="Apple-style-span" face="Courier"> | |Elem: bf |</font></div><div><font class="Apple-style-span" face="Courier"> | *-------|--*</font></div><div><font class="Apple-style-span" face="Courier"> +------------------------------+</font></div><div><br></div><div>As you can see, the forward pointer of the sentinel is never</div><div>accessed (save the asserts verifying the legal operator++ invocations),</div><div>because it is forbidden to dereference the end() iterator.</div><div><br></div><div>So my old patch eliminated the forward pointer from the sentinel, thus</div><div>shrinking ilist by a pointer and thus also Obj.</div><div><br></div><div>I do not think that this is a risky issue, and there is still a bunch of time</div><div>till 2.6 branches. The patch proper is just about 10 lines of code and can</div><div>be backed out with ease if something goes wrong (which I do not expect).</div></div><div><br><blockquote type="cite"><div><br></div><div>> This is how I came up with LLVM_COMPACT_SENTINELS in configure. It</div><div>> defaults to 1 if</div><div>> in Release-Asserts mode. Now if clients include the same config.h and</div><div>> link against the</div><div>> Release-Asserts libraries, there will be no trouble. I guess this is</div><div>> the primary use case.</div><div><br></div><div>How about just making it a autoconf switch decoupled from either </div><div>existing setting? Why exactly don't you want this for non-assert mode?</div></blockquote><div><br></div><div>If I make an autoconf setting for this, then nobody will use it, which entails</div><div>lost benefits.</div><div><br></div><div>I think the automatism of "Release mode without asserts" is exactly when I</div><div>want the benefits of this optimization. But I am not religious about this.</div><div><br></div><div>Cheers,</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>Gabor<br></div><br><blockquote type="cite"><div><br></div><div>-Chris</div></blockquote></div> </div></blockquote></div><br></body></html>