[PATCH] Prefer packed attribute when conflicts with volatile

[Jie Zhang]

Hi Mark,

Thanks for your review!

On 10/21/2010 12:57 PM, Mark Mitchell wrote:
> On 10/20/2010 6:58 PM, Jie Zhang wrote:
>
>> No problem. I can try someone else. The patch changes code in tree ->
>> RTL. You are listed as the maintainer for RTL optimizers. I know it's
>> not perfectly match but you and Richard Henderson are recommended for
>> reviewing my patch on IRC. Maybe we should have a maintainer for expand
>> pass?
>
> I think the patch makes sense, but I will pick a few nits. :-)
>
> Algorithmically, why do we have to call extract_fixed_bit_field twice
> (once with final clear and once with it set)?  Can't we issue the
> warning and then just keep going?  Or, in the worst case, do a goto to
> the top of the function?  Callers shouldn't have to handle this
> final/not-final distinction.
>

extract_fixed_bit_field first calculates the best mode, then does works 
based on that mode. When it gets to the point that packed attribute 
conflicts with volatile, it need recalculate the best mode and restart 
works. So we cannot issue the warning and then just keep going.

After more thought, now I think it should be enough to just call 
extract_split_bit_field for such cases. extract_split_bit_field will 
call into extract_fixed_bit_field. It has the same effect of restarting 
extract_fixed_bit_field. The resulted patch is simplified.

> Also, as a point of style, new boolean parameters should be declared as
> "bool", not as "int".  So, for example, "extract_fixed_bit_field" should
> have "packedp" as "bool".
>
Done.

> (Also, although it's not specified as a standard GNU style, I like to write:
>
>    ..., /*packedp=*/true, ...
>
> when passing cryptic boolean arguments to functions; otherwise, it's
> confusing to look at:
>
>    f (true, false, 1, 0, false)
>
> and work out what it's doing.  You can decide whether you want to use
> that style or not.)
>
That's good practice. But only there are very few occurrences in GCC. So 
I would just keep current code.

> Please resubmit with those changes.
>
The new patch is attached. Test is still going. Is it OK if the test 
result is good?


Regards,
-- 
Jie Zhang
CodeSourcery
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gcc-packed-override-strict-volatile-bitfields-2.diff
Type: text/x-patch
Size: 11127 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20101021/28b867c1/attachment.bin>