[4/8] Add bit_field_mode_iterator

get_best_mode has various checks to decide what counts as an acceptable
bitfield mode.  It actually has two copies of them, with slightly different
alignment checks:

  MIN (unit, BIGGEST_ALIGNMENT) > align

vs.

  unit <= MIN (align, BIGGEST_ALIGNMENT)

The second looks more correct, since we can't necessarily guarantee
larger alignments than BIGGEST_ALIGNMENT in all cases.

This patch adds a new iterator class that can be used to walk through
the modes, and rewrites get_best_mode to use it.  I kept the existing
checks with two changes:

- bitregion_start is now tested independently of bitregion_end
- MAX_FIXED_MODE_SIZE is used as a limit even if a bitregion is defined

It shouldn't make any difference in practice, but both changes felt
more in keeping with the documentation of bitregion_start and
MAX_FIXED_MODE_SIZE, and the next patch wants the bitregion_end
test to be separate from bitregion_start.

The behaviour of the Sequent i386 compiler probably isn't the
issue it once was, but that's also dealt with in the next patch.

Tested as described in the covering note.  OK to install?

Richard


gcc/
	* machmode.h (bit_field_mode_iterator): New class.
	(get_best_mode): Change final parameter to bool.
	* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
	(bit_field_mode_iterator::next_mode): New functions, split out from...
	(get_best_mode): ...here.  Change final parameter to bool.
	Use bit_field_mode_iterator.

> get_best_mode has various checks to decide what counts as an acceptable
> bitfield mode.  It actually has two copies of them, with slightly different
> alignment checks:
> 
>   MIN (unit, BIGGEST_ALIGNMENT) > align
> 
> vs.
> 
>   unit <= MIN (align, BIGGEST_ALIGNMENT)
> 
> The second looks more correct, since we can't necessarily guarantee
> larger alignments than BIGGEST_ALIGNMENT in all cases.

Under the assumption that integer modes really require maximal alignment, i.e. 
whatever BIGGEST_ALIGNMENT is, I agree.

> This patch adds a new iterator class that can be used to walk through
> the modes, and rewrites get_best_mode to use it.  I kept the existing
> checks with two changes:
> 
> - bitregion_start is now tested independently of bitregion_end

The comments needs to be updated then.

> - MAX_FIXED_MODE_SIZE is used as a limit even if a bitregion is defined

This makes sense I think.

> It shouldn't make any difference in practice, but both changes felt
> more in keeping with the documentation of bitregion_start and
> MAX_FIXED_MODE_SIZE, and the next patch wants the bitregion_end
> test to be separate from bitregion_start.
> 
> The behaviour of the Sequent i386 compiler probably isn't the
> issue it once was, but that's also dealt with in the next patch.
> 
> Tested as described in the covering note.  OK to install?
> 
> Richard
> 
> 
> gcc/
> 	* machmode.h (bit_field_mode_iterator): New class.
> 	(get_best_mode): Change final parameter to bool.
> 	* stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator)
> 	(bit_field_mode_iterator::next_mode): New functions, split out from...
> 	(get_best_mode): ...here.  Change final parameter to bool.
> 	Use bit_field_mode_iterator.

This looks good to me, modulo:

> +  volatilep_ (volatilep), count_ (0)
> +{
> +  if (bitregion_end_)
> +    bitregion_end_ += 1;
> +}

IMO this is confusing.  I think bitregion_end/bitregion_end_ should have a 
consistent meaning.


> +/* Calls to this function return successively larger modes that can be used
> +   to represent the bitfield.  Return true if another bitfield mode is +  
> available, storing it in *OUT_MODE if so.  */
> +
> +bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode)

'bool' on its own line I think.


I find the interface a bit awkward though.  Can't we model it on the existing 
iterators in basic-block.h or tree-flow.h?  get_best_mode would be written:

  FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
			  bitregion_start, bitregion_end,
			  align, volatilep)
    {
	if (largest_mode != VOIDmode
	    && GET_MODE_SIZE (mode) > GET_MODE_SIZE (largest_mode)
	  break;

	if (iter.prefer_smaller_modes ())
	  return mode;

	widest_mode = mode; 
    }

  return widest_mode;

and the implementation entirely hidden.

On 11/13/2012 04:41 AM, Eric Botcazou wrote:
> I find the interface a bit awkward though.  Can't we model it on the existing 
> iterators in basic-block.h or tree-flow.h?  get_best_mode would be written:
> 
>   FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
> 			  bitregion_start, bitregion_end,
> 			  align, volatilep)

Now that we're in C++, I think we should be using iterators that are much
more in the style of libstdc++.  I agree that the .next interface used here
is a bit confusing.

I'd expect to see something like

  for (bit_field_mode_iterator i(x,y,z); i ; ++i)
    {
      enum machine_mode mode = *i;
      ...
    }

For 4.9, I expect that someone can slowly purge our FOR_EACH_* macros...


r~

> Now that we're in C++, I think we should be using iterators that are much
> more in the style of libstdc++.  I agree that the .next interface used here
> is a bit confusing.
> 
> I'd expect to see something like
> 
>   for (bit_field_mode_iterator i(x,y,z); i ; ++i)
>     {
>       enum machine_mode mode = *i;
>       ...
>     }

I pondered on that for half an hour. :-)  But the amount of stuff you need to 
write to make it work in this particular case will make the implementation 
convoluted and bloated for no obvious gains IMO.

Eric Botcazou <ebotcazou@adacore.com> writes:
>> Now that we're in C++, I think we should be using iterators that are much
>> more in the style of libstdc++.  I agree that the .next interface used here
>> is a bit confusing.
>> 
>> I'd expect to see something like
>> 
>>   for (bit_field_mode_iterator i(x,y,z); i ; ++i)
>>     {
>>       enum machine_mode mode = *i;
>>       ...
>>     }
>
> I pondered on that for half an hour. :-)  But the amount of stuff you need to 
> write to make it work in this particular case will make the implementation 
> convoluted and bloated for no obvious gains IMO.

Yeah, I'd also thought about doing it as a "proper" iterator, and came
to the same conclusion.  Although I was thinking of:

  bit_field_def def (blah...);
  for (bit_field_def::mode_iterator i = def.modes_begin();
       i != def.modes_end();
       ++i)
    {
      enum machine_mode mode = *i;
      ...
    }

Then presumably we'd need all the other iterator gloss too
(value_type, etc.).

I hadn't thought about an operator bool terminator.  I agree that's
probably simpler, but do any libstdc++ classes have the same thing?
It doesn't feel any more standard than the "while (get_more)" idiom to me,
but that's probably just my ignorance of C++.

My problem with:

  FOR_EACH_BITFIELD_MODE (mode, iter, bitsize, bitpos,
			  bitregion_start, bitregion_end,
			  align, volatilep)

is that there are too many parameters to make it obvious what we're
actually iterating over.  I.e. these FOR_EACH_* macros tend to have a
combination of three things: inputs, "internal" iterator state that
nevertheless needs to be defined separately in order to avoid surprising
name clashes, and the iterator variable itself.  When there's a lot of
parameters, it's not always obvious which is which.

Also, it makes it harder to implement loops like:

  setup
  if (get first mode)
    if (want to search for more)
      while (more modes...)

which is the structure used in patch 7.

Admittedly both "problems" apply to some of the existing FOR_EACH_*
macros too, so there's definitely a consistency argument in favour of
doing this anyway.

"next" was supposed to be "find and return another mode" rather than "++".
Did you think it was confusing because "next" sounded too much like the latter?
Or was it the whole "while (get more)" thing you didn't like?

Maybe it would be better to forget about using classes here and have
something like (with "bit_field_def def"):

    mode = smallest_bit_field_mode (&def, params...);
    while (mode != MAX_MACHINE_MODE)
      {
        ...
        mode = wider_bit_field_mode (&def, mode);
      }

Richard

On 11/15/2012 04:10 AM, Richard Sandiford wrote:
> "next" was supposed to be "find and return another mode" rather than "++".
> Did you think it was confusing because "next" sounded too much like the latter?

I wasn't keen on "next" being find-and-return, though I didn't
actually find it confusing.  And perhaps rather than bikeshed
this too much now, we should table this for revision in 4.9...

> I hadn't thought about an operator bool terminator.  I agree that's
> probably simpler, but do any libstdc++ classes have the same thing?
> It doesn't feel any more standard than the "while (get_more)" idiom to me,
> but that's probably just my ignorance of C++.

... when we can attack all the iterators.

No, you're right that operator bool as a terminator isn't standard.
Though for many purposes it seems better than the "!= fake_end_object"
semantics that we'd have to use otherwise.

That's a discussion that we should have generally as we find our 
feet with C++ in GCC.

Unless Eric has any strong objections, I think this patch is ok.
And thus the entire patch set, as I havn't seen anything else that
raises a red flag.


r~

Richard Henderson <rth@redhat.com> writes:
> On 11/15/2012 04:10 AM, Richard Sandiford wrote:
>> "next" was supposed to be "find and return another mode" rather than "++".
>> Did you think it was confusing because "next" sounded too much like
>> the latter?
>
> I wasn't keen on "next" being find-and-return, though I didn't
> actually find it confusing.  And perhaps rather than bikeshed
> this too much now, we should table this for revision in 4.9...
>
>> I hadn't thought about an operator bool terminator.  I agree that's
>> probably simpler, but do any libstdc++ classes have the same thing?
>> It doesn't feel any more standard than the "while (get_more)" idiom to me,
>> but that's probably just my ignorance of C++.
>
> ... when we can attack all the iterators.
>
> No, you're right that operator bool as a terminator isn't standard.
> Though for many purposes it seems better than the "!= fake_end_object"
> semantics that we'd have to use otherwise.
>
> That's a discussion that we should have generally as we find our 
> feet with C++ in GCC.
>
> Unless Eric has any strong objections, I think this patch is ok.
> And thus the entire patch set, as I havn't seen anything else that
> raises a red flag.

Thanks.  Committed with the changes Eric asked for after retesting
on x86_64-linux-gnu, powerpc64-linux-gnu and mipsisa64-elf.

Richard