<div dir="ltr"><div dir="ltr">On Wed, Mar 11, 2020 at 12:42 PM Serge Pavlov <<a href="mailto:sepavloff@gmail.com">sepavloff@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">It is a matter of taste, but for me it looks strange to develop complex and error-prone system to save 7 bits at expense of readability and maintainability. My observations show that clang AST consumes much less memory than llvm objects.</div></blockquote><div>I agree, and would be happy if we had a design without these scarce bits, but AFAICS we don't. In this environment, 5 bits are quite a lot for FP flexibility, and I think the complexity of reducing this is small and isolated (rounding + exceptions together fit in 4 bits). But I ask about the domain because maybe there's a simple but smaller model I can't really infer this myself, both rounding and exceptions seem essentially unused at present.</div><div>(Note that the cost here is not 7 bits per node but 63 - once the bits available in Stmt are full the stmt subclass needs to gain a member, and its alignment is 8 *bytes*)</div><div><br></div><div>Use cases vary, and many tools deal with large ASTs but don't use LLVM codegen at all.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>FPOption could be shared between user using something like shared_ptr, but it means expenses of 64 bit for a pointer. Don't know if it makes sense...<br></div></div></blockquote><div>As you say, I don't think this saves anything, unless we can stop storing the pointer in BinaryOperator.</div><div><br></div><div>Cheers, Sam</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div><br><div><br clear="all"><div><div dir="ltr">Thanks,<br>--Serge<br></div></div><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">sammccall added a comment.<br>
<br>
This patch increased the used size of BinaryOperator by 5 bits.<br>
Those bits were all padding, but now BinaryOperatorBitfields is completely full at 32 bits and we can't add any new bits to Stmt without increasing BinaryOperator by 8 bytes. (See D75443 <<a href="https://reviews.llvm.org/D75443" rel="noreferrer" target="_blank">https://reviews.llvm.org/D75443</a>> and D54526 <<a href="https://reviews.llvm.org/D54526" rel="noreferrer" target="_blank">https://reviews.llvm.org/D54526</a>> for the optimization this would revert).<br>
<br>
To squeeze in the new bit I'm planning to suggest squeezing getInt() to 7 bits (it encodes 3x2x5x3 = 90 states, so this is possible) but I'm not really familiar with this domain - if many of the 90 states are not possible it'd probably be useful to have some more bits back :-)<br>
<br>
<br>
Repository:<br>
  rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D65994/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D65994/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D65994" rel="noreferrer" target="_blank">https://reviews.llvm.org/D65994</a><br>
<br>
<br>
<br>
</blockquote></div>
</blockquote></div></div>