On Wed, 25 May 2011, Jan Hubicka wrote:
> > > *************** lto_output_tree (struct output_block *ob
> > > *** 1401,1407 ****
> > > will instantiate two different nodes for the same object. */
> > > output_record_start (ob, LTO_tree_pickle_reference);
> > > output_uleb128 (ob, ix);
> > > ! output_uleb128 (ob, lto_tree_code_to_tag (TREE_CODE (expr)));
> > > }
> > > else if (lto_stream_as_builtin_p (expr))
> > > {
> > > --- 1399,1405 ----
> > > will instantiate two different nodes for the same object. */
> > > output_record_start (ob, LTO_tree_pickle_reference);
> > > output_uleb128 (ob, ix);
> > > ! output_record_start (ob, lto_tree_code_to_tag (TREE_CODE (expr)));
> >
> > I'd prefer lto_output_enum here as we don't really start a new output
> > record but just emit something for a sanity check.
>
> OK, I wondered what is cleaner, will update the patch.
> > > + /* Output VAL into OBS and verify it is in range MIN...MAX that is
> > > supposed
> > > + to be compile time constant.
> > > + Be host independent, limit range to 31bits. */
> > > +
> > > + static inline void
> > > + lto_output_int_in_range (struct lto_output_stream *obs,
> > > + HOST_WIDE_INT min,
> > > + HOST_WIDE_INT max,
> > > + HOST_WIDE_INT val)
> > > + {
> > > + HOST_WIDE_INT range = max - min;
> > > +
> > > + gcc_checking_assert (val >= min && val <= max && range > 0
> > > + && range < 0x7fffffff);
> > > +
> > > + val -= min;
> > > + lto_output_1_stream (obs, val & 255);
> > > + if (range >= 0xff)
> > > + lto_output_1_stream (obs, (val << 8) & 255);
> > > + if (range >= 0xffff)
> > > + lto_output_1_stream (obs, (val << 16) & 255);
> > > + if (range >= 0xffffff)
> > > + lto_output_1_stream (obs, (val << 24) & 255);
> >
> > so you didn't want to create a bitpack_pack_int_in_range and use
> > bitpacks for enums? I suppose for some of the remaining cases
> > packing them into existing bitpacks would be preferable?
>
> Well, in my TODO list is to have both. Where we don't bitpatck enums with
> other values (that is the most common case of enums) this way we produce less
> overhead and have extra sanity check that the bits unused by enum are really
> 0.
>
> I guess final API should have both lto_output_enum and
> lto_bitpack_output_enum.
> I don't really care if the first have the implementation above or just
> creates its
> own bitpack to handle the value.
Ok.
> > > + {
> > > + HOST_WIDE_INT range = max - min;
> > > + HOST_WIDE_INT val = lto_input_1_unsigned (ib);
> > > +
> > > + gcc_checking_assert (range > 0);
> >
> > The assert doesn't match the one during output.
>
> Hmm, OK, will match.
Patch is ok with the changes.
Thanks,
Richard.
