diff mbox

[2/N] Add dump_flags_type<E> for handling of suboptions.

Message ID c079b2b5-66e0-fcac-a543-b9221bc313de@suse.cz
State New
Headers show

Commit Message

Martin Liška May 5, 2017, 10:44 a.m. UTC
Hi.

This one is more interesting as it implements hierarchical option parsing
and as a first step I implemented that for optgroup suboptions.

Next candidates are dump_option_value_info and obviously my primary motivation:
dump_option_value_info.

I'm expecting feedback for implementation I've decided to come up with.
Patch has been tested.

Thanks,
Martin

Comments

Martin Sebor May 12, 2017, 5:42 p.m. UTC | #1
On 05/05/2017 04:44 AM, Martin Liška wrote:
> Hi.
>
> This one is more interesting as it implements hierarchical option parsing
> and as a first step I implemented that for optgroup suboptions.

I haven't gone through the rest of the patches so I could be
missing some context.  But I have a few observations about and
suggestions for the new dump_option_node (and to a smaller extent,
dump_flags_type) class template. (Feel free to disregard whatever
doesn't make sense.)

First, since dump_option_node's public member functions (including
the only ctor) are defined in dumpfile.c it seems that the template
definition doesn't really need to be provided in the header. If
that's correct, it would reduce coupling and might avoid bloat to
only declare the bare minimum needed to use the type in other .c
files, and define the rest in the one .c file where the complete
type is actually needed.

As far as I can see, there is just one instantiation of the template
in the patch, in dumpfile.c:

+  typedef dump_option_node<optgroup_types> node;

If there are no other existing instantiations and no others are
anticipated, it could even be a plain class (further reducing
complexity and bloat).

Finally, the template ctor takes a const char* and stores it
in a member, and the implicit copy ctor and assignment operator
copy the underlying vec class.  That means that the string
argument must outlive the constructed object, which is not
typically expected.  IIUC, vec is not safely copy-assignable
or copy-constructible (i.e., has undefined behavior).  At
a minimum, it would be appropriate to document these
constraints.  Removing them or preventing copy construction
and assignment to avoid getting bit by it would be even better.

For dump_flags_type, none of the members need to be explicitly
declared inline or mention the <E> template argument list.
operator&(dump_flags_type) should be declared const.  Compound
assignment operators should return a reference to *this  (to
behave as closely to the native operators).  The exclusive OR
operator and compound assignment are missing and should probably
be provided for completeness, even if they aren't needed.  The
m_mask member should probably be private for better encapsulation
(and operator uint64_t() const provided to provide implicit
conversion to that type, or perhaps one converting to E's
underlying type; that should also obviate the need for operator
bool).

Martin
Martin Sebor May 15, 2017, 2:23 a.m. UTC | #2
On 05/12/2017 11:42 AM, Martin Sebor wrote:
> On 05/05/2017 04:44 AM, Martin Liška wrote:
>> Hi.
>>
>> This one is more interesting as it implements hierarchical option parsing
>> and as a first step I implemented that for optgroup suboptions.
>
> I haven't gone through the rest of the patches so I could be
> missing some context.  But I have a few observations about and
> suggestions for the new dump_option_node (and to a smaller extent,
> dump_flags_type) class template. (Feel free to disregard whatever
> doesn't make sense.)
>
> First, since dump_option_node's public member functions (including
> the only ctor) are defined in dumpfile.c it seems that the template
> definition doesn't really need to be provided in the header. If
> that's correct, it would reduce coupling and might avoid bloat to
> only declare the bare minimum needed to use the type in other .c
> files, and define the rest in the one .c file where the complete
> type is actually needed.
>
> As far as I can see, there is just one instantiation of the template
> in the patch, in dumpfile.c:
>
> +  typedef dump_option_node<optgroup_types> node;
>
> If there are no other existing instantiations and no others are
> anticipated, it could even be a plain class (further reducing
> complexity and bloat).
>
> Finally, the template ctor takes a const char* and stores it
> in a member, and the implicit copy ctor and assignment operator
> copy the underlying vec class.  That means that the string
> argument must outlive the constructed object, which is not
> typically expected.  IIUC, vec is not safely copy-assignable
> or copy-constructible (i.e., has undefined behavior).  At
> a minimum, it would be appropriate to document these
> constraints.  Removing them or preventing copy construction
> and assignment to avoid getting bit by it would be even better.
>
> For dump_flags_type, none of the members need to be explicitly
> declared inline or mention the <E> template argument list.
> operator&(dump_flags_type) should be declared const.  Compound
> assignment operators should return a reference to *this  (to
> behave as closely to the native operators).  The exclusive OR
> operator and compound assignment are missing and should probably
> be provided for completeness, even if they aren't needed.  The
> m_mask member should probably be private for better encapsulation
> (and operator uint64_t() const provided to provide implicit
> conversion to that type, or perhaps one converting to E's
> underlying type; that should also obviate the need for operator
> bool).

One other comment/suggestion I meant to add but forgot:

It's best to make binary operators like operator| non-members.
That way, they are symmetric in terms of implicit conversions
of their operands.  E.g., with dump_flags_type which defines
a conversion ctor from uint64_t (whether or not that's a good
approach is a separate topic), it's expected that if

   dump_flags_type () | uint64_t (1)

is valid, then

   uint64_t (1) | dump_flags_type ()

is valid as well.  With operator| being a dump_flags_type member
the former would be valid but the latter would not be.  In other
words, the general rule is to only define member functions for
the smallest set of operations that cannot be non-members (like
compound assignments).

Martin
Martin Liška May 15, 2017, 9:39 a.m. UTC | #3
Hello.

Thanks Martin for feedback! After I spent quite some time with fiddling with
the options, I'm not convinced we should convert options to more hierarchical
structure. There's description:

1) -fopt-info is used to dump optimization options. One can pick both verbosity
(note, optimization, all) and an optimization (ipa, inline, vec,...). Thus said
it's probably not a candidate for hierarchical options?

2) -fdump-pass_name-... as mentioned by Nathan is combination of verbosity
(graph, note, verbose, details) and specific type of options (VOPS, RHS_ONLY, UID,..).

There's a complete list and suggestion how we can move it to more hierarchical ordering:

#define TDF_ADDRESS
#define TDF_SLIM
#define TDF_RAW
#define TDF_DETAILS
#define TDF_STATS
#define TDF_BLOCKS
#define TDF_VOPS
#define TDF_LINENO
#define TDF_UID
#define TDF_TREE - remove & replace with DI_kind
#define TDF_RTL - remove & replace with DI_kind
#define TDF_IPA - remove & replace with DI_kind
#define TDF_STMTADDR - merge with TDF_ADDRESS
#define TDF_GRAPH
#define TDF_MEMSYMS
#define TDF_DIAGNOSTIC - merge with TDF_DETAILS
#define TDF_VERBOSE - merge with TDF_DETAILS
#define TDF_RHS_ONLY
#define TDF_ASMNAME
#define TDF_EH
#define TDF_NOUID
#define TDF_ALIAS
#define TDF_ENUMERATE_LOCALS
#define TDF_CSELIB
#define TDF_SCEV
#define TDF_COMMENT - remove and dump ';; ' unconditionally
#define TDF_GIMPLE

and more hierarchical ordering can be:

#define TDF_ADDRESS
#define TDF_SLIM
#define TDF_RAW
#define TDF_DETAILS
#define TDF_STATS
#define TDF_BLOCKS
#define TDF_LINENO
#define TDF_UID
#define TDF_GRAPH
#define TDF_ASMNAME
#define TDF_NOUID
#define TDF_ENUMERATE_LOCALS

#define TDF_GIMPLE
#define TDF_GIMPLE_FE - GIMPLE front-end
#define TDF_GIMPLE_VOPS
#define TDF_GIMPLE_EH
#define TDF_GIMPLE_ALIAS
#define TDF_GIMPLE_SCEV
#define TDF_GIMPLE_MEMSYMS
#define TDF_GIMPLE_RHS_ONLY

#define TDF_RTL
#define TDF_RTL_CSELIB

I already discussed that with Richi, but I would like to receive a feedback about TDF_ clean
and about -fopt-info.

Thanks,
Martin
Nathan Sidwell May 15, 2017, 11:33 a.m. UTC | #4
Martin,
thanks for the write up.

On 05/15/2017 05:39 AM, Martin Liška wrote:
> Thanks Martin for feedback! After I spent quite some time with fiddling with > the options, I'm not convinced we should convert options to more 
hierarchical> structure. There's description:
> 1) -fopt-info is used to dump optimization options. One can pick both verbosity
> (note, optimization, all) and an optimization (ipa, inline, vec,...). Thus said
> it's probably not a candidate for hierarchical options?

I've never used this option, so have no comment (too lazy to figure out 
if it told me anything I didn't already see in a dump I was groveling over).

> 2) -fdump-pass_name-... as mentioned by Nathan is combination of verbosity
> (graph, note, verbose, details) and specific type of options (VOPS, RHS_ONLY, UID,..).
> 
> There's a complete list and suggestion how we can move it to more hierarchical ordering:
> 
> #define TDF_ADDRESS
> #define TDF_SLIM
> #define TDF_RAW
> #define TDF_DETAILS
> #define TDF_STATS
> #define TDF_BLOCKS
> #define TDF_VOPS
> #define TDF_LINENO
> #define TDF_UID
#define TDF_LANG is now a thing too. it should be DI_kind too
> #define TDF_TREE - remove & replace with DI_kind
> #define TDF_RTL - remove & replace with DI_kind
> #define TDF_IPA - remove & replace with DI_kind
> #define TDF_STMTADDR - merge with TDF_ADDRESS
> #define TDF_GRAPH
> #define TDF_MEMSYMS
> #define TDF_DIAGNOSTIC - merge with TDF_DETAILS
> #define TDF_VERBOSE - merge with TDF_DETAILS
> #define TDF_RHS_ONLY
> #define TDF_ASMNAME
> #define TDF_EH
> #define TDF_NOUID
> #define TDF_ALIAS
> #define TDF_ENUMERATE_LOCALS
> #define TDF_CSELIB
> #define TDF_SCEV
> #define TDF_COMMENT - remove and dump ';; ' unconditionally
> #define TDF_GIMPLE

Looks a good start.

1) The TDF_UID and TDF_NOUID options seem to be inverses of each other. 
Can't we just ditch the latter?

2) We might want to distinguish between enabling dump information that 
is useful to us gcc developers (TDF_DETAILS, say), and that that would 
be useful to end users trying to figure out why some random loop isn't 
being optimized in (say TDF_DIAGNOSTIC).  But if we can't define a 
sensible way of distinguishing then I'm all for not making the distinction.

> and more hierarchical ordering can be:
> 
> #define TDF_ADDRESS
> #define TDF_SLIM
> #define TDF_RAW
> #define TDF_DETAILS
> #define TDF_STATS
> #define TDF_BLOCKS
> #define TDF_LINENO
> #define TDF_UID
> #define TDF_GRAPH
> #define TDF_ASMNAME
> #define TDF_NOUID
> #define TDF_ENUMERATE_LOCALS

It'd be nice to name TDF_ENUMERATE_LOCALS without the second _ to avoid 
confusion with the hierarchy you discuss below?  (perhaps TDF_LOCALS?)

I like the idea of naming flags specific to a particular kind of dump 
with the name of that kind of dump.  We do have a mismatch between 
DI_TREE and TDF_GIMPLE though -- is there something sensible we could do 
there?

> #define TDF_GIMPLE
> #define TDF_GIMPLE_FE - GIMPLE front-end

How might this differ from a new  -fdump-lang-original?  I.e.
(1) why is it a dump-modifier flag, rather than a dump in its own right
(2) if we do need it, name it TDF_GIMPLE_LANG

> #define TDF_GIMPLE_VOPS
> #define TDF_GIMPLE_EH
> #define TDF_GIMPLE_ALIAS
> #define TDF_GIMPLE_SCEV
> #define TDF_GIMPLE_MEMSYMS
> #define TDF_GIMPLE_RHS_ONLY
> 
> #define TDF_RTL

How does this differ from the current TDF_RTL meaning?  Is it implying 
'TDF_RTL_ALL'? (same question about TDF_GIMPLE).

> #define TDF_RTL_CSELIB

nathan
Martin Liška May 15, 2017, 12:04 p.m. UTC | #5
On 05/15/2017 01:33 PM, Nathan Sidwell wrote:
> Martin,
> thanks for the write up.
> 
> On 05/15/2017 05:39 AM, Martin Liška wrote:
>> Thanks Martin for feedback! After I spent quite some time with fiddling with > the options, I'm not convinced we should convert options to more 
> hierarchical> structure. There's description:
>> 1) -fopt-info is used to dump optimization options. One can pick both verbosity
>> (note, optimization, all) and an optimization (ipa, inline, vec,...). Thus said
>> it's probably not a candidate for hierarchical options?
> 
> I've never used this option, so have no comment (too lazy to figure out if it told me anything I didn't already see in a dump I was groveling over).

Hi.

Thanks for feedback, I'll mention -fopt-info later in this email.

> 
>> 2) -fdump-pass_name-... as mentioned by Nathan is combination of verbosity
>> (graph, note, verbose, details) and specific type of options (VOPS, RHS_ONLY, UID,..).
>>
>> There's a complete list and suggestion how we can move it to more hierarchical ordering:
>>
>> #define TDF_ADDRESS
>> #define TDF_SLIM
>> #define TDF_RAW
>> #define TDF_DETAILS
>> #define TDF_STATS
>> #define TDF_BLOCKS
>> #define TDF_VOPS
>> #define TDF_LINENO
>> #define TDF_UID
> #define TDF_LANG is now a thing too. it should be DI_kind too
>> #define TDF_TREE - remove & replace with DI_kind
>> #define TDF_RTL - remove & replace with DI_kind
>> #define TDF_IPA - remove & replace with DI_kind
>> #define TDF_STMTADDR - merge with TDF_ADDRESS
>> #define TDF_GRAPH
>> #define TDF_MEMSYMS
>> #define TDF_DIAGNOSTIC - merge with TDF_DETAILS
>> #define TDF_VERBOSE - merge with TDF_DETAILS
>> #define TDF_RHS_ONLY
>> #define TDF_ASMNAME
>> #define TDF_EH
>> #define TDF_NOUID
>> #define TDF_ALIAS
>> #define TDF_ENUMERATE_LOCALS
>> #define TDF_CSELIB
>> #define TDF_SCEV
>> #define TDF_COMMENT - remove and dump ';; ' unconditionally
>> #define TDF_GIMPLE
> 
> Looks a good start.
> 
> 1) The TDF_UID and TDF_NOUID options seem to be inverses of each other. Can't we just ditch the latter?

One is used to paper over UIDs in order to preserve -fdebug-compare (I believe). And the second one
is used to dump UIDd basically for *_DECL. As any of these is not default, both make sense.

> 
> 2) We might want to distinguish between enabling dump information that is useful to us gcc developers (TDF_DETAILS, say), and that that would be useful to end users trying to figure out why some random loop isn't being optimized in (say TDF_DIAGNOSTIC).  But if we can't define a sensible way of distinguishing then I'm all for not making the distinction.

That's probably motivation behind -fopt-info, which should represent "Optimization dumps", readable by user.
To be honest, just a part of optimizations can be found in these files (vectorization and loop optimization).
The rest lives in normal -fdump-xyz*. Maybe this can be opportunity to clean it up?

> 
>> and more hierarchical ordering can be:
>>
>> #define TDF_ADDRESS
>> #define TDF_SLIM
>> #define TDF_RAW
>> #define TDF_DETAILS
>> #define TDF_STATS
>> #define TDF_BLOCKS
>> #define TDF_LINENO
>> #define TDF_UID
>> #define TDF_GRAPH
>> #define TDF_ASMNAME
>> #define TDF_NOUID
>> #define TDF_ENUMERATE_LOCALS
> 
> It'd be nice to name TDF_ENUMERATE_LOCALS without the second _ to avoid confusion with the hierarchy you discuss below?  (perhaps TDF_LOCALS?)

Yep, works for me.

> 
> I like the idea of naming flags specific to a particular kind of dump with the name of that kind of dump.  We do have a mismatch between DI_TREE and TDF_GIMPLE though -- is there something sensible we could do there?
> 
>> #define TDF_GIMPLE
>> #define TDF_GIMPLE_FE - GIMPLE front-end

As we have couple of dump flags used just for GIMPLE dumps, my idea was to give them common predecessor (TDF_GIMPLE).
Which explains why current TDF_GIMPLE (GIMPLE FE) needs to be renamed.
> 
> How might this differ from a new  -fdump-lang-original?  I.e.
> (1) why is it a dump-modifier flag, rather than a dump in its own right

Because you can use it for all gimple/tree passes to produce input for GIMPLE FE.

> (2) if we do need it, name it TDF_GIMPLE_LANG

Can be done that.

> 
>> #define TDF_GIMPLE_VOPS
>> #define TDF_GIMPLE_EH
>> #define TDF_GIMPLE_ALIAS
>> #define TDF_GIMPLE_SCEV
>> #define TDF_GIMPLE_MEMSYMS
>> #define TDF_GIMPLE_RHS_ONLY
>>
>> #define TDF_RTL
> 
> How does this differ from the current TDF_RTL meaning?  Is it implying 'TDF_RTL_ALL'? (same question about TDF_GIMPLE).

Yes, -fdump-tree-xyz-rtl would be equal to -fdump-tree-xyz-rtl-all.

Martin

> 
>> #define TDF_RTL_CSELIB
> 
> nathan
Nathan Sidwell May 15, 2017, 12:24 p.m. UTC | #6
On 05/15/2017 08:04 AM, Martin Liška wrote:
> On 05/15/2017 01:33 PM, Nathan Sidwell wrote:

>> 1) The TDF_UID and TDF_NOUID options seem to be inverses of each other. Can't we just ditch the latter?
> 
> One is used to paper over UIDs in order to preserve -fdebug-compare (I believe). And the second one
> is used to dump UIDd basically for *_DECL. As any of these is not default, both make sense.

Might I suggest we rename at least one of them then?

>> How does this differ from the current TDF_RTL meaning?  Is it implying 'TDF_RTL_ALL'? (same question about TDF_GIMPLE).
> 
> Yes, -fdump-tree-xyz-rtl would be equal to -fdump-tree-xyz-rtl-all.

I wonder if we can name things to be a little clearer?  Here you're 
applying an rtl modifier to a tree dump.  I find that jarring, given we 
have rtl dumps themselves. (I don't have a good suggestion right now).

Given a blank sheet of paper, the current 'TDF_tree' dumps should really 
be 'TDF_gimple' dumps, so we'd have lang/ipa/gimple/rtl kinds of dumps. 
Such a renaming may be an unacceptable amount of churn though.

nathan
Martin Liška May 15, 2017, 1:06 p.m. UTC | #7
On 05/15/2017 02:24 PM, Nathan Sidwell wrote:
> On 05/15/2017 08:04 AM, Martin Liška wrote:
>> On 05/15/2017 01:33 PM, Nathan Sidwell wrote:
> 
>>> 1) The TDF_UID and TDF_NOUID options seem to be inverses of each other. Can't we just ditch the latter?
>>
>> One is used to paper over UIDs in order to preserve -fdebug-compare (I believe). And the second one
>> is used to dump UIDd basically for *_DECL. As any of these is not default, both make sense.
> 
> Might I suggest we rename at least one of them then?

Yep, it's doable.

> 
>>> How does this differ from the current TDF_RTL meaning?  Is it implying 'TDF_RTL_ALL'? (same question about TDF_GIMPLE).
>>
>> Yes, -fdump-tree-xyz-rtl would be equal to -fdump-tree-xyz-rtl-all.
> 
> I wonder if we can name things to be a little clearer?  Here you're applying an rtl modifier to a tree dump.  I find that jarring, given we have rtl dumps themselves. (I don't have a good suggestion right now).

Sorry, that's confusing example.

> 
> Given a blank sheet of paper, the current 'TDF_tree' dumps should really be 'TDF_gimple' dumps, so we'd have lang/ipa/gimple/rtl kinds of dumps. Such a renaming may be an unacceptable amount of churn though.

Well, I would prefer to introduce new enum for kind of dump:
https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01033.html

and TDF_GIMPLE_* would really reflect to a dump suboption which is used by GIMPLE pretty printer related functions.

Martin

> 
> nathan
>
Nathan Sidwell May 15, 2017, 2:12 p.m. UTC | #8
On 05/15/2017 09:06 AM, Martin Liška wrote:

>> Given a blank sheet of paper, the current 'TDF_tree' dumps should really be 'TDF_gimple' dumps, so we'd have lang/ipa/gimple/rtl kinds of dumps. Such a renaming may be an unacceptable amount of churn though.
> 
> Well, I would prefer to introduce new enum for kind of dump:
> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01033.html

Right, I understand that.  My point is that it might be confusing to 
users of the dump machinery (i.e. me), at the command-line level where 
'rtl' means different things in different contexts.  And we have 'tree' 
dumps that dump gimple and 'lang' dumps that also (can) dump trees.

We have a bunch of gimple optimization passes, but call the dumpers 
'tree'.  I know how we ended up here, but it seems confusing.

nathan
Martin Liška May 15, 2017, 2:21 p.m. UTC | #9
On 05/15/2017 04:12 PM, Nathan Sidwell wrote:
> On 05/15/2017 09:06 AM, Martin Liška wrote:
> 
>>> Given a blank sheet of paper, the current 'TDF_tree' dumps should really be 'TDF_gimple' dumps, so we'd have lang/ipa/gimple/rtl kinds of dumps. Such a renaming may be an unacceptable amount of churn though.
>>
>> Well, I would prefer to introduce new enum for kind of dump:
>> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01033.html
> 
> Right, I understand that.  My point is that it might be confusing to users of the dump machinery (i.e. me), at the command-line level where 'rtl' means different things in different contexts.  And we have 'tree' dumps that dump gimple and 'lang' dumps that also (can) dump trees.

Right. To be honest, originally I was convinced about positive impact of hierarchical options. But changing names of dump suboptions will bring
inconvenience for current developers of GCC (who mainly use it). And I also noticed that one can write -fdump-tree-ifcvt-stats-blocks-details,
a combination of multiple suboptions. Which makes it even more complex :)

That said, I'm not inclining to that. Then it's questionable whether to encapsulate masking enum to a class?

Martin

> 
> We have a bunch of gimple optimization passes, but call the dumpers 'tree'.  I know how we ended up here, but it seems confusing.
> 
> nathan
>
Richard Biener May 16, 2017, 1:42 p.m. UTC | #10
On Mon, May 15, 2017 at 4:21 PM, Martin Liška <mliska@suse.cz> wrote:
> On 05/15/2017 04:12 PM, Nathan Sidwell wrote:
>> On 05/15/2017 09:06 AM, Martin Liška wrote:
>>
>>>> Given a blank sheet of paper, the current 'TDF_tree' dumps should really be 'TDF_gimple' dumps, so we'd have lang/ipa/gimple/rtl kinds of dumps. Such a renaming may be an unacceptable amount of churn though.
>>>
>>> Well, I would prefer to introduce new enum for kind of dump:
>>> https://gcc.gnu.org/ml/gcc-patches/2017-05/msg01033.html
>>
>> Right, I understand that.  My point is that it might be confusing to users of the dump machinery (i.e. me), at the command-line level where 'rtl' means different things in different contexts.  And we have 'tree' dumps that dump gimple and 'lang' dumps that also (can) dump trees.
>
> Right. To be honest, originally I was convinced about positive impact of hierarchical options. But changing names of dump suboptions will bring
> inconvenience for current developers of GCC (who mainly use it). And I also noticed that one can write -fdump-tree-ifcvt-stats-blocks-details,
> a combination of multiple suboptions. Which makes it even more complex :)
>
> That said, I'm not inclining to that. Then it's questionable whether to encapsulate masking enum to a class?

So replying to the last mail in this thread.

Originally the motivation was to get more bits when we disallow some
flags for some categories (lang, tree, rtl, ipa).  Now given
that we have 32bits free and that Nathans TDF_lang stuff should add
missing hierarchy the only user-visible change I'd do is
to disallow for example

-fdump-rtl-vops

or

-fdump-tree-note

with a diagnostic.  That allows us to allocate overlapping bits.  Like
if we move TDF_* to a .def file and have

DEFFLAG(EH, GIMPLE|RTL)
DEFFLAG(NOTE, INFO)
DEFFLAG(SCEV, GIMPLE)

etc. and some "intelligent" generator that allocates the bits.

Now, given we have 32bit free the pressure to do this is quite low
(and I usally dislike changes that are just there
for the sake of changes).

Given the introduction of -lang, which I appreciate, there's the
opportunity to do better than currently, esp. with
respect to maintainance of dump flags and the machinery.

I realize this doesn't really answer whether we want a class or a enum
or a uint64_t for dump_flags.  A class would
be most forward-looking.  An enum for the bit number then works well
(I dislike "sparse" enums), so operator|, etc.
would do the missing 1<<

Richard.

> Martin
>
>>
>> We have a bunch of gimple optimization passes, but call the dumpers 'tree'.  I know how we ended up here, but it seems confusing.
>>
>> nathan
>>
>
diff mbox

Patch

From fcb78a3d07f3043766f27f73038f313f914b3976 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Fri, 5 May 2017 11:31:18 +0200
Subject: [PATCH 2/2] Add dump_flags_type<E> for handling of suboptions.

gcc/ChangeLog:

2017-05-05  Martin Liska  <mliska@suse.cz>

	* dumpfile.c (dump_option_node::initialize): New function.
	(dump_option_node::initialize_masks): Likewise.
	(dump_option_node::parse): Likewise.
	(gcc::dump_manager::dump_manager): Initialize options.
	(dump_switch_p_1): Use the new parser.
	(initialize_options): New function.
	(opt_info_switch_p_1): Use the new parser.
	(opt_info_switch_p): Use new dump_flags_type<E> type.
	* dumpfile.h (struct dump_option_node): New struct.
	(struct dump_flags_type): Likewise.
	(enum optgroup_types): New enum type.
	(struct dump_file_info): Change type of optgroup_flags.
---
 gcc/dumpfile.c | 141 ++++++++++++++++++++++++++++++++++++++++----------
 gcc/dumpfile.h | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 255 insertions(+), 46 deletions(-)

diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 907ded3695f..82c4fc9d4ff 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -135,17 +135,78 @@  static const struct dump_option_value_info optinfo_verbosity_options[] =
   {NULL, 0}
 };
 
-/* Flags used for -fopt-info groups.  */
-static const struct dump_option_value_info optgroup_options[] =
-{
-  {"ipa", OPTGROUP_IPA},
-  {"loop", OPTGROUP_LOOP},
-  {"inline", OPTGROUP_INLINE},
-  {"omp", OPTGROUP_OMP},
-  {"vec", OPTGROUP_VEC},
-  {"optall", OPTGROUP_ALL},
-  {NULL, 0}
-};
+template <typename E>
+dump_option_node<E>::dump_option_node (const char *name, E enum_value):
+  m_name (name), m_enum_value (enum_value), m_children (), m_mask (0)
+{
+}
+
+template <typename E>
+void
+dump_option_node<E>::initialize (uint64_t *mask_translation)
+{
+  memset (mask_translation, 0, sizeof (uint64_t) * OPT_MASK_SIZE);
+  unsigned current = 0;
+  initialize_masks (&current, mask_translation);
+}
+
+template <typename E>
+uint64_t
+dump_option_node<E>::initialize_masks (unsigned *current,
+				       uint64_t *mask_translation)
+{
+  if (m_children.is_empty ())
+    {
+      gcc_assert (*current < OPT_MASK_SIZE);
+      m_mask = 1 << *current;
+      *current += 1;
+    }
+  else
+    {
+      uint64_t combined = 0;
+      for (unsigned i = 0; i < m_children.length (); i++)
+	combined |= m_children[i]->initialize_masks (current, mask_translation);
+
+      m_mask = combined;
+    }
+
+  mask_translation[m_enum_value] = m_mask;
+  return m_mask;
+}
+
+template <typename E>
+uint64_t
+dump_option_node<E>::parse (const char *token)
+{
+  char *s = xstrdup (token);
+  uint64_t r = parse (s);
+  free (s);
+
+  return r;
+}
+
+template <typename E>
+uint64_t
+dump_option_node<E>::parse (char *token)
+{
+  if (token == NULL)
+    return m_mask;
+
+  if (strcmp (token, "all") == 0)
+  {
+    token = strtok (NULL, "-");
+    return token == NULL ? m_mask : 0;
+  }
+
+  for (unsigned i = 0; i < m_children.length (); i++)
+    if (strcmp (m_children[i]->m_name, token) == 0)
+    {
+      token = strtok (NULL, "-");
+      return m_children[i]->parse (token);
+    }
+
+  return 0;
+}
 
 gcc::dump_manager::dump_manager ():
   m_next_dump (FIRST_AUTO_NUMBERED_DUMP),
@@ -153,6 +214,7 @@  gcc::dump_manager::dump_manager ():
   m_extra_dump_files_in_use (0),
   m_extra_dump_files_alloced (0)
 {
+  initialize_options ();
 }
 
 gcc::dump_manager::~dump_manager ()
@@ -174,12 +236,14 @@  gcc::dump_manager::~dump_manager ()
       XDELETEVEC (const_cast <char *> (dfi->alt_filename));
     }
   XDELETEVEC (m_extra_dump_files);
+
+  delete (optgroup_options);
 }
 
 unsigned int
 gcc::dump_manager::
 dump_register (const char *suffix, const char *swtch, const char *glob,
-	       dump_flags_t flags, int optgroup_flags,
+	       dump_flags_t flags, optgroup_dump_flags_t  optgroup_flags,
 	       bool take_ownership)
 {
   int num = m_next_dump++;
@@ -716,8 +780,8 @@  dump_enable_all (dump_flags_t flags, const char *filename)
 
 int
 gcc::dump_manager::
-opt_info_enable_passes (int optgroup_flags, dump_flags_t flags,
-			const char *filename)
+opt_info_enable_passes (optgroup_dump_flags_t optgroup_flags,
+			dump_flags_t flags, const char *filename)
 {
   int n = 0;
   size_t i;
@@ -808,7 +872,7 @@  dump_switch_p_1 (const char *arg, struct dump_file_info *dfi, bool doglob)
 	if (strlen (option_ptr->name) == length
 	    && !memcmp (option_ptr->name, ptr, length))
           {
-            flags |= option_ptr->value;
+	    flags |= option_ptr->value;
 	    goto found;
           }
 
@@ -865,15 +929,36 @@  dump_switch_p (const char *arg)
   return any;
 }
 
+void
+gcc::dump_manager::
+initialize_options ()
+{
+  /* Initialize optgroup options.  */
+  typedef dump_option_node<optgroup_types> node;
+
+  optgroup_options = new node (NULL, OPTGROUP_NONE);
+  optgroup_options->register_suboption (new node ("ipa", OPTGROUP_IPA));
+  optgroup_options->register_suboption (new node ("loop", OPTGROUP_LOOP));
+  optgroup_options->register_suboption (new node ("inline", OPTGROUP_INLINE));
+  optgroup_options->register_suboption (new node ("omp", OPTGROUP_OMP));
+  optgroup_options->register_suboption (new node ("vec", OPTGROUP_VEC));
+  optgroup_options->register_suboption (new node ("other", OPTGROUP_OTHER));
+
+  optgroup_options->initialize (optgroup_dump_flags_t::m_mask_translation);
+}
+
 /* Parse ARG as a -fopt-info switch and store flags, optgroup_flags
    and filename.  Return non-zero if it is a recognized switch.  */
 
 static int
-opt_info_switch_p_1 (const char *arg, dump_flags_t *flags, int *optgroup_flags,
-                     char **filename)
+opt_info_switch_p_1 (const char *arg, dump_flags_t *flags,
+		     optgroup_dump_flags_t *optgroup_flags,
+		     char **filename)
 {
   const char *option_value;
   const char *ptr;
+  optgroup_dump_flags_t f;
+  gcc::dump_manager *dumps = g->get_dumps ();
 
   option_value = arg;
   ptr = option_value;
@@ -885,6 +970,7 @@  opt_info_switch_p_1 (const char *arg, dump_flags_t *flags, int *optgroup_flags,
   if (!ptr)
     return 1;       /* Handle '-fopt-info' without any additional options.  */
 
+
   while (*ptr)
     {
       const struct dump_option_value_info *option_ptr;
@@ -909,17 +995,16 @@  opt_info_switch_p_1 (const char *arg, dump_flags_t *flags, int *optgroup_flags,
 	if (strlen (option_ptr->name) == length
 	    && !memcmp (option_ptr->name, ptr, length))
           {
-            *flags |= option_ptr->value;
+	    *flags |= option_ptr->value;
 	    goto found;
-          }
+	  }
 
-      for (option_ptr = optgroup_options; option_ptr->name; option_ptr++)
-	if (strlen (option_ptr->name) == length
-	    && !memcmp (option_ptr->name, ptr, length))
-          {
-            *optgroup_flags |= option_ptr->value;
-	    goto found;
-          }
+      f = dumps->get_optgroup_options ()->parse (ptr);
+      if (f)
+	{
+	  *optgroup_flags |= f;
+	  goto found;
+	}
 
       if (*ptr == '=')
         {
@@ -948,7 +1033,7 @@  int
 opt_info_switch_p (const char *arg)
 {
   dump_flags_t flags;
-  int optgroup_flags;
+  optgroup_dump_flags_t optgroup_flags;
   char *filename;
   static char *file_seen = NULL;
   gcc::dump_manager *dumps = g->get_dumps ();
@@ -971,7 +1056,7 @@  opt_info_switch_p (const char *arg)
   if (!flags)
     flags = MSG_OPTIMIZED_LOCATIONS;
   if (!optgroup_flags)
-    optgroup_flags = OPTGROUP_ALL;
+    optgroup_flags = optgroup_dump_flags_t::get_all ();
 
   return dumps->opt_info_enable_passes (optgroup_flags, flags, filename);
 }
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index 76183e3eede..67da7003331 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -90,20 +90,134 @@  enum tree_dump_index
 #define MSG_ALL         (MSG_OPTIMIZED_LOCATIONS | MSG_MISSED_OPTIMIZATION \
                          | MSG_NOTE)
 
+/* Dump option node is a tree structure that implements
+   parsing of suboptions and provides mapping between a given enum type E
+   and unsigned integer masks that are encapsulated in dump_flags_type type.  */
 
-/* Flags to control high-level -fopt-info dumps.  Usually these flags
-   define a group of passes.  An optimization pass can be part of
-   multiple groups.  */
-#define OPTGROUP_NONE        (0)
-#define OPTGROUP_IPA         (1 << 1)   /* IPA optimization passes */
-#define OPTGROUP_LOOP        (1 << 2)   /* Loop optimization passes */
-#define OPTGROUP_INLINE      (1 << 3)   /* Inlining passes */
-#define OPTGROUP_OMP         (1 << 4)   /* OMP (Offloading and Multi
-					   Processing) transformations */
-#define OPTGROUP_VEC         (1 << 5)   /* Vectorization passes */
-#define OPTGROUP_OTHER       (1 << 6)   /* All other passes */
-#define OPTGROUP_ALL	     (OPTGROUP_IPA | OPTGROUP_LOOP | OPTGROUP_INLINE \
-                              | OPTGROUP_OMP | OPTGROUP_VEC | OPTGROUP_OTHER)
+template <typename E>
+struct dump_option_node
+{
+  /* Constructor.  */
+  dump_option_node (const char *name, E enum_value);
+
+  /* Initialize hierarchy and fill up a MASK_TRANLATION table.  */
+  void initialize (uint64_t *mask_translation);
+
+  /* Parse a given option string and return mask.  */
+  uint64_t parse (const char *token);
+
+  /* Register a SUBOPTION for a dump option node.  */
+  void register_suboption (dump_option_node<E> *suboption)
+  {
+    m_children.safe_push (suboption);
+  }
+
+private:
+  /* Initialize masks for internal nodes.  CURRENT is a counter with first
+     free mask.  MASK_TRANSLATION is table that is filled up.  */
+  uint64_t initialize_masks (unsigned *current, uint64_t *mask_translation);
+
+  /* Parse a given option string and return mask.  */
+  uint64_t parse (char *token);
+
+  /* Name of the option.  */
+  const char *m_name;
+
+  /* Enum value of the option.  */
+  E m_enum_value;
+
+  /* Children options.  */
+  vec<dump_option_node *> m_children;
+
+  /* Mask that represents the option.  */
+  uint64_t m_mask;
+};
+
+/* Size of possible valid leaf options.  */
+#define OPT_MASK_SIZE (CHAR_BIT * sizeof (uint64_t))
+
+/* Dump flags type represents a set of selected options for
+   a given enum type E.  */
+
+template <typename E>
+struct dump_flags_type
+{
+  /* Constructor.  */
+  dump_flags_type<E> (): m_mask (0)
+  {}
+
+  /* Constructor for a MASK.  */
+  dump_flags_type<E> (uint64_t mask): m_mask (mask)
+  {}
+
+  /* Constructor for a enum value E.  */
+  dump_flags_type<E> (E enum_value)
+  {
+    gcc_checking_assert ((unsigned)enum_value <= OPT_MASK_SIZE);
+    m_mask = m_mask_translation[enum_value];
+  }
+
+  /* OR operator for OTHER dump_flags_type.  */
+  inline void operator|= (dump_flags_type other)
+  {
+    m_mask |= other.m_mask;
+  }
+
+  /* AND operator for OTHER dump_flags_type.  */
+  inline void operator&= (dump_flags_type other)
+  {
+    m_mask &= other.m_mask;
+  }
+
+  /* AND operator for OTHER dump_flags_type.  */
+  inline bool operator& (dump_flags_type other)
+  {
+    return m_mask & other.m_mask;
+  }
+
+  /* Bool operator that is typically used to test whether an option is set.  */
+  inline operator bool () const
+  {
+    return m_mask;
+  }
+
+  /* Return mask that represents all selected options.  */
+  static inline dump_flags_type get_all ()
+  {
+    return m_mask_translation[0];
+  }
+
+  /* Initialize.  */
+  static dump_flags_type parse (char *option);
+
+  /* Selected mask of options.  */
+  uint64_t m_mask;
+
+  /* Translation table between enum values and masks.  */
+  static uint64_t m_mask_translation[OPT_MASK_SIZE];
+};
+
+/* Flags used for -fopt-info groups.  */
+
+enum optgroup_types
+{
+  OPTGROUP_NONE,
+  OPTGROUP_IPA,
+  OPTGROUP_LOOP,
+  OPTGROUP_INLINE,
+  OPTGROUP_OMP,
+  OPTGROUP_VEC,
+  OPTGROUP_OTHER,
+  OPTGROUP_COUNT
+};
+
+template<typename E>
+uint64_t
+dump_flags_type<E>::m_mask_translation[OPT_MASK_SIZE];
+
+/* Dump flags type for optgroup_types enum type.  */
+
+typedef dump_flags_type<optgroup_types> optgroup_dump_flags_t;
 
 /* Dump flags type.  */
 
@@ -120,7 +234,7 @@  struct dump_file_info
   FILE *pstream;                /* pass-specific dump stream  */
   FILE *alt_stream;             /* -fopt-info stream */
   dump_flags_t pflags;		/* dump flags */
-  int optgroup_flags;           /* optgroup flags for -fopt-info */
+  optgroup_dump_flags_t optgroup_flags; /* optgroup flags for -fopt-info */
   int alt_flags;                /* flags for opt-info */
   int pstate;                   /* state of pass-specific stream */
   int alt_state;                /* state of the -fopt-info stream */
@@ -187,7 +301,7 @@  public:
      SUFFIX, SWTCH, and GLOB. */
   unsigned int
   dump_register (const char *suffix, const char *swtch, const char *glob,
-		 dump_flags_t flags, int optgroup_flags,
+		 dump_flags_t flags, optgroup_dump_flags_t  optgroup_flags,
 		 bool take_ownership);
 
   /* Return the dump_file_info for the given phase.  */
@@ -232,6 +346,12 @@  public:
   const char *
   dump_flag_name (int phase) const;
 
+  /* Return optgroup_types dump options.  */
+  dump_option_node<optgroup_types> *get_optgroup_options ()
+  {
+    return optgroup_options;
+  }
+
 private:
 
   int
@@ -244,8 +364,10 @@  private:
   dump_enable_all (dump_flags_t flags, const char *filename);
 
   int
-  opt_info_enable_passes (int optgroup_flags, dump_flags_t flags,
-			  const char *filename);
+  opt_info_enable_passes (optgroup_dump_flags_t optgroup_flags,
+			  dump_flags_t flags, const char *filename);
+
+  void initialize_options ();
 
 private:
 
@@ -255,12 +377,14 @@  private:
   size_t m_extra_dump_files_in_use;
   size_t m_extra_dump_files_alloced;
 
+  /* Dump option node for optgroup_types enum.  */
+  dump_option_node<optgroup_types> *optgroup_options;
+
   /* Grant access to dump_enable_all.  */
   friend bool ::enable_rtl_dump_file (void);
 
   /* Grant access to opt_info_enable_passes.  */
   friend int ::opt_info_switch_p (const char *arg);
-
 }; // class dump_manager
 
 } // namespace gcc
-- 
2.12.2