On Wed, Mar 11, 2020 at 12:42 PM Serge Pavlov <sepavl...@gmail.com> wrote:
> 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. > 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. (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*) Use cases vary, and many tools deal with large ASTs but don't use LLVM codegen at all. 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... > As you say, I don't think this saves anything, unless we can stop storing the pointer in BinaryOperator. Cheers, Sam > > > Thanks, > --Serge > > > On Wed, Mar 11, 2020 at 6:30 PM Sam McCall via Phabricator < > revi...@reviews.llvm.org> wrote: > >> sammccall added a comment. >> >> This patch increased the used size of BinaryOperator by 5 bits. >> 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 < >> https://reviews.llvm.org/D75443> and D54526 < >> https://reviews.llvm.org/D54526> for the optimization this would revert). >> >> 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 :-) >> >> >> Repository: >> rG LLVM Github Monorepo >> >> CHANGES SINCE LAST ACTION >> https://reviews.llvm.org/D65994/new/ >> >> https://reviews.llvm.org/D65994 >> >> >> >>
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
