[cfe-commits] [PATCH] Driver: Unify CC1Options.td and Options.td

James Molloy james.molloy at arm.com
Tue Jan 31 01:42:04 PST 2012


Hi Chandler,

Could I possibly please ping you for a followup review to this?

Cheers,

James

-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu
[mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of James Molloy
Sent: 27 January 2012 10:47
To: 'Chandler Carruth'
Cc: Joerg Sonnenberger; cfe-commits at cs.uiuc.edu
Subject: Re: [cfe-commits] [PATCH] Driver: Unify CC1Options.td and
Options.td

Hi Chandler,

Thanks for the comments.

> Really cool! So, I assume one of the next big things on this path is to
lift the current serialization / deserialization / parsing logic inside of
the Frontend into the Driver as well, making the Frontend simply accept
option structs and interfaces? That's been a long-standing goal of mine.
Essentially, the options shouldn't escape the driver.

The way I see it, the options should be serialized and deserialized by a
CompilerInvocation object. Whether that lives in libFrontend.a or
libDriver.a is really moot from my POV, but yes it can be moved wholesale
into the Driver.

> The other big thing I think to simplify Tools.cpp is to come up with
common patterns for the flags that go through a round of translation when
going > from the 'clang' CLI to the CC1 CLI, and to specify these patterns
in the TableGen spec for the flags. That where you see this going? I just
want to > get a good shared picture of how this will look now, so that we
don't have too many false starts. =]

So the current patch deals with the trivial "round trip" of parsing a flag
in the driver and emitting that *exact same flag* for the frontend. Other
flags that are different in the 'clang' CLI to the CC1 CLI, can be dealt
with via Aliases I'd have thought? I'm not massively worried about them.

What I am worried about is keeping the default options all in one place,
along with the [de]serialization. I'm also currently concerned about the
lack of orthogonality between -fFOO and -fno-FOO options - having the
negation specified separately seems to be heading for disaster where there's
no inverse for an option. I'd like to have the OptParser deal with negation
itself, if possible.

> Actually, it would be awesome to have a comprehensive (even if initially
mechanically generated) test for all of the pure forwarding behavior, and
lack of pure forwarding. That way each time a flag is added we know to go
add it to one of the test cases. Is that do-able?

So you're suggesting a test suite that would test whether each option is
actually being used/acted on? I'm not sure - with the current patch we
essentially remove most forwarding. Only a CompilerInvocation will parse
most flags intended for cc1, so the driver need not worry about them or
forward them. What I'm suggesting is changing Clang::constructJob() to be
essentially:

   CompilerInvocation CI;
   CompilerInvocation::CreateFromArgs(Args, CI);
   // Modify CI here based on stuff only the driver knows - host/target
triple, isystem, -I etc.
   CI.Serialize(CmdArgs);

So, no forwarding and no defaults for the majority of options.

> I have one request to change the way you've structured the result. I'd
like to pull all of the pure-forwarded CC1 options down into a single file.
What I'm imagining conceptually is the following. We have essentially two
CLIs (and hopefully eventually a third):

It'd take a bit of work to go through and find all options the driver isn't
using itself - can I please do that as a followup patch?

> If this is the direction you're thinking as well, I'd like to move the CC1
bits down into the CC1Options.td file, and then (likely after your patch)
rename Options.td to something better (GCCOptions.td doesn't seem quite
right, but not sure what is). The goal being to start separating in the
tables the CC1 interface from the compat wrappers.

Yeah, although I admit that my priority is more "making Tools.cpp and
ToolChains.cpp manageable and not ridden with copypasta" and sorting out the
cross-compilation story via a target database. So the argument translation
is low on my radar (but still there).

I agree with you though, totally.

> I've not looked at some of the details of the patch, I'll try to get to
that tomorrow.. The only thing that looked even a little odd was the -O
handling -- might just want comments or something there.

Yeah the reason that changed was that CC1 had special options for Os/Oz, and
the driver handles them all via Joined<"-O">.

Cheers,

James

From: Chandler Carruth [mailto:chandlerc at google.com] 
Sent: 27 January 2012 10:20
To: James Molloy
Cc: cfe-commits at cs.uiuc.edu; Joerg Sonnenberger
Subject: Re: [PATCH] Driver: Unify CC1Options.td and Options.td

On Fri, Jan 27, 2012 at 1:44 AM, James Molloy <james.molloy at arm.com> wrote:
Hi,

The attached patch changes the way CC1 options are handled, unifying the
generated enums for both driver and CC1. This:
 * Will remove duplicated CC1/Driver options, removing around 140
duplicated options.
 * Stops the need to add new compiler options in two places.
 * Paves the way for the driver to construct a CompilerInvocation instead
of manually translating options and adding defaults. When this is done it
will reduce the code in Tools.cpp drastically.

Really cool! So, I assume one of the next big things on this path is to lift
the current serialization / deserialization / parsing logic inside of the
Frontend into the Driver as well, making the Frontend simply accept option
structs and interfaces? That's been a long-standing goal of mine.
Essentially, the options shouldn't escape the driver.

The other big thing I think to simplify Tools.cpp is to come up with common
patterns for the flags that go through a round of translation when going
from the 'clang' CLI to the CC1 CLI, and to specify these patterns in the
TableGen spec for the flags. That where you see this going? I just want to
get a good shared picture of how this will look now, so that we don't have
too many false starts. =]

There are only two minor functionality changes (apart from that most flags
that were not correctly forwarded by the driver now get forwarded):

It would be awesome to get some more test coverage for these...

Actually, it would be awesome to have a comprehensive (even if initially
mechanically generated) test for all of the pure forwarding behavior, and
lack of pure forwarding. That way each time a flag is added we know to go
add it to one of the test cases. Is that do-able?


This is the first step in having only one place that arguments are parsed
for Clang.

The patch is large mainly because of all the flags that needed to be
de-duplicated (and I also copied over the HelpText from CC1Options.td in
these cases because Options.td was sorely lacking in help text).

Yea, the help text is a real sore spot for the actual options we expose.

I have one request to change the way you've structured the result. I'd like
to pull all of the pure-forwarded CC1 options down into a single file. What
I'm imagining conceptually is the following. We have essentially two CLIs
(and hopefully eventually a third):

1) CC1's CLI -- Every functional switch has to exist at least here.
2) GCC-compat CLI
3) (eventually) cl.exe compat CLI

For simplicity and sanity, I think its healthy to make CC1 be as much a
subset of #2 (or in theory #3, but that ship sailed long ago) as possible
just to reduce the size of the interface. The result is that CC1 has all of
the GCC-compat options that make enough sense to use directly, and al of the
Clang-specific options in a GCC-style formulation. The GCC-compat layer adds
on top of CC1 all the GCC flags that have too confusing of names to deal
with internally or don't map directly onto CC1 flag, as well as any logic
necessary to translate into CC1. #2 will also suppress any CC1 flags from #1
that if exposed would break some compat aspect (I don't know what thes
ewould be, but in theory...). Finally, the #3 layer will be essentially the
same as #2, but with essentially everything from CC1 suppressed, and a
complete wrapper with translation logic provided.

If this is the direction you're thinking as well, I'd like to move the CC1
bits down into the CC1Options.td file, and then (likely after your patch)
rename Options.td to something better (GCCOptions.td doesn't seem quite
right, but not sure what is). The goal being to start separating in the
tables the CC1 interface from the compat wrappers.

Thoughts?

I've not looked at some of the details of the patch, I'll try to get to that
tomorrow.. The only thing that looked even a little odd was the -O handling
-- might just want comments or something there.




_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits








More information about the cfe-commits mailing list