On Fri, May 31, 2013 at 4:12 PM, David Malcolm <[email protected]> wrote:
> On Tue, 2013-05-28 at 12:30 -0600, Jeff Law wrote:
>> On 05/28/2013 11:00 AM, David Malcolm wrote:
>> > On Tue, 2013-05-28 at 06:39 -0600, Jeff Law wrote:
>> >> On 05/25/2013 07:02 AM, David Malcolm wrote:
>> >>> I can think of three approaches to "cfun":
>> >>> (a) status quo: a global variable, with macros to prevent direct
>> >>> assignment, and an API for changing cfun.
>> >>> (b) have a global "context" or "universe" object, and put cfun in
>> >>> there (perhaps with tricks to be able to make this a singleton in a
>> >>> non-library build, optimizing away the context lookups somehow
>> >>> - see [2] for discussion on this)
>> >>> (c) go through all of the places where cfun is used, and somehow ensure
>> >>> that they're passed in the data they need. Often it's not the
>> >>> function that's used, but its cfg.
>> >> I'd think B or C is going to be the way to go here. B may also be an
>> >> intermediate step towards C.
>> >>
>> >>>
>> >>> One part of the puzzle is that various header files in the build define
>> >>> macros that reference the "cfun" global, e.g.:
>> >>>
>> >>> #define n_basic_blocks (cfun->cfg->x_n_basic_blocks)
>> >>>
>> >>> This one isn't in block caps, which might mislead a new contributor into
>> >>> thinking it's a variable, rather than a macro, so there may be virtue in
>> >>> removing these macros for that reason alone. (I know that these confused
>> >>> me for a while when I first started writing my plugin) [3]
>> >> There's a few of these that have crept in over the years.
>> >> n_basic_blocks used to be a global variable. At some point it was
>> >> stuffed into cfun, but it was decided not to go back and fix all the
>> >> references -- possibly due to not wanting to fix the overly long lines
>> >> after the mechanical change.
>> >
>> > If a mechanical change could fix the overly-long lines as it went along,
>> > would such a change be acceptable now?
>> Probably. However, I would advise against trying to pack too much into
>> a single patch.
>>
>> So one possibility to move forward would be a single patch which removes
>> the n_basic_blocks macro and fixes all the uses along with their
>> overly-long lines. It's simple, obvious and very self-contained.
>
> Thanks. I'm attaching such a patch.
>
> Successful 3-stage build against r199533; "make check" shows the same
> results as an unpatched 3-stage build of that same revision (both on
> x86_64-unknown-linux-gnu).
>
> OK for trunk?
>
> Should I continue with individual patches to remove each macro, in turn?
Sorry for taking so long to look at this. I'd prefer instead of hunks like
@@ -2080,7 +2080,7 @@ reorder_basic_blocks (void)
gcc_assert (current_ir_type () == IR_RTL_CFGLAYOUT);
- if (n_basic_blocks <= NUM_FIXED_BLOCKS + 1)
+ if (cfun->cfg->n_basic_blocks <= NUM_FIXED_BLOCKS + 1)
return;
set_edge_can_fallthru_flag ();
to use
if (n_basic_blocks_for_function (cfun) <= NUM_FIXED_BLOCKS + 1)
so we have a single way over the compiler to access n_basic_blocks.
Ok with that change, and ok with following up with patches for the
other individual macros, replacing them with existing _for_function
variants.
Btw, I'm also ok with shortening these macros to use _for_fn instead
at the same time, adjusting the very few existing invocations of them.
Thanks,
Richard.
> Dave
