> On 09/19/14 22:04, Jan Hubicka wrote: > >Hi, > >int_bit_position is used by ipa-devirt's type walking code. It is currently > >a bottleneck > >since I introduced speculation into contextes (I plan to solve this by > >changing the > >way i cache results). But this patch seems to make sense anyway: we do not > >need to go > >through folding: > >tree > >bit_from_pos (tree offset, tree bitpos) > >{ > > if (TREE_CODE (offset) == PLUS_EXPR) > > offset = size_binop (PLUS_EXPR, > > fold_convert (bitsizetype, TREE_OPERAND (offset, > > 0)), > > fold_convert (bitsizetype, TREE_OPERAND (offset, > > 1))); > > else > > offset = fold_convert (bitsizetype, offset); > > return size_binop (PLUS_EXPR, bitpos, > > size_binop (MULT_EXPR, offset, bitsize_unit_node)); > >} > > > >Because all the code cares only about constant offsets, we do not need to go > >through fold_convert, > >because all the codes go via int_bit_position that already expects result to > >be host wide int, > >it seems to make sense to implement quick path for that. > > > >Bootstrap/regtest x86_64 in progress, OK? > > > >Honza > > > > * stor-layout.c (int_bit_from_pos): New function. > > * stor-layout.h (int_bit_from_pos): Declare. > > * tree.c (int_bit_from_pos): Use it. > Just as a note to anyone else that peeks at this code, tree_to_shwi > will verify the nodes are constant during a checking build. > > I'd consider a different name for the function that somehow > indicates the inputs are constants. > > jeff > > Please consider an assert or other checking code to ensure that > OFFSET and BITPOS are constants. Oh, I see that tree_to_shwi will > get that checking when it > > Jeff
Yep, tree_to_shwi will check it. The old code did generic expression folding and called tree_to_shwi on result, so the only difference is that old code will accept unfolded expressions that miraculously folds into constant. I think it is bug to have those in IL especially on places we do not expect variable offsets. Based on that observation, I think we can also drop handling of PLUS_EXPR in this case as follows. Concerning the function, it has documented in toplevel comment that parameter must be constant or it crashes, so I think it is fine. Conerning name, I am open for renaming, but we have those int_* variants in quite few copies, so I can do that incrementally (see int_byte_position and related functions in stor layout). I am testing the following simplified (and inline) variant. Perhaps we could do similar stuff for other int_* accessors even if they do not sit on hot paths in my test, just for the sake of code size. Honza Index: tree.c =================================================================== --- tree.c (revision 215421) +++ tree.c (working copy) @@ -2831,16 +2831,6 @@ bit_position (const_tree field) return bit_from_pos (DECL_FIELD_OFFSET (field), DECL_FIELD_BIT_OFFSET (field)); } - -/* Likewise, but return as an integer. It must be representable in - that way (since it could be a signed value, we don't have the - option of returning -1 like int_size_in_byte can. */ - -HOST_WIDE_INT -int_bit_position (const_tree field) -{ - return tree_to_shwi (bit_position (field)); -} /* Return the byte position of FIELD, in bytes from the start of the record. This is a tree of type sizetype. */ Index: tree.h =================================================================== --- tree.h (revision 215421) +++ tree.h (working copy) @@ -3877,10 +3877,20 @@ extern tree size_in_bytes (const_tree); extern HOST_WIDE_INT int_size_in_bytes (const_tree); extern HOST_WIDE_INT max_int_size_in_bytes (const_tree); extern tree bit_position (const_tree); -extern HOST_WIDE_INT int_bit_position (const_tree); extern tree byte_position (const_tree); extern HOST_WIDE_INT int_byte_position (const_tree); +/* Like bit_position, but return as an integer. It must be representable in + that way (since it could be a signed value, we don't have the + option of returning -1 like int_size_in_byte can. */ + +static inline HOST_WIDE_INT int_bit_position (const_tree field) +{ + return tree_to_shwi (DECL_FIELD_OFFSET (field)) * BITS_PER_UNIT + + tree_to_shwi (DECL_FIELD_BIT_OFFSET (field)); +} + + #define sizetype sizetype_tab[(int) stk_sizetype] #define bitsizetype sizetype_tab[(int) stk_bitsizetype] #define ssizetype sizetype_tab[(int) stk_ssizetype]