Issue 1803403002: Subzero. Flags refactoring.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+626 lines, -1197 lines)			Patch
M	src/IceBrowserCompileServer.h	View	1 2	3 chunks	+1 line, -5 lines	0 comments	Download
M	src/IceBrowserCompileServer.cpp	View	1 2 3 4	2 chunks	+3 lines, -4 lines	0 comments	Download
M	src/IceCfg.cpp	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/IceClFlags.h	View	1 2 3 4 5 6	3 chunks	+91 lines, -457 lines	0 comments	Download
M	src/IceClFlags.cpp	View	1 2 3 4 5 6 7 8	2 chunks	+140 lines, -549 lines	0 comments	Download
A	src/IceClFlags.def	View	1 2 3 4 5 6 7 8	1 chunk	+338 lines, -0 lines	0 comments	Download
D	src/IceClFlagsExtra.h	View	1	1 chunk	+0 lines, -123 lines	0 comments	Download
M	src/IceCompileServer.cpp	View	1 2 3 4 5	6 chunks	+19 lines, -22 lines	0 comments	Download
M	src/IceCompiler.h	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
M	src/IceCompiler.cpp	View	1 2	4 chunks	+7 lines, -8 lines	0 comments	Download
M	src/IceELFObjectWriter.cpp	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	src/IceGlobalContext.h	View	1 2 3 4 5	1 chunk	+0 lines, -1 line	0 comments	Download
M	src/IceGlobalContext.cpp	View	1 2 3 4 5	5 chunks	+3 lines, -5 lines	0 comments	Download
M	src/IceGlobalInits.h	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -1 line	0 comments	Download
M	src/IceTargetLowering.cpp	View	1 2 3 4 5	3 chunks	+7 lines, -4 lines	0 comments	Download
M	src/IceTargetLoweringARM32.cpp	View	1 2 3 4 5	5 chunks	+5 lines, -5 lines	0 comments	Download
M	src/IceTargetLoweringMIPS32.cpp	View	1 2 3 4 5	4 chunks	+4 lines, -4 lines	0 comments	Download
M	src/IceTargetLoweringX86BaseImpl.h	View	1 2 3 4 5	3 chunks	+3 lines, -3 lines	0 comments	Download
M	src/IceTypeConverter.h	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -1 line	0 comments	Download

Messages

Total messages: 11 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Jim Stichnoth

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.cpp File src/IceClFlags.cpp (right): https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.cpp#newcode25 src/IceClFlags.cpp:25: #pragma clang diagnostic ignored "-Wredundant-move" this shouldn't be needed ...

4 years, 9 months ago (2016-03-18 19:49:35 UTC) #3

John

4 years, 9 months ago (2016-03-21 14:03:36 UTC) #4

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.cpp#new...
src/IceClFlags.cpp:25: #pragma clang diagnostic ignored "-Wredundant-move"
On 2016/03/18 19:49:35, stichnot wrote:
> this shouldn't be needed

Done.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:1: //===- subzero/src/IceClFlags.h - Cl Flags for translation
------*- C++ -*-===//
On 2016/03/18 19:49:35, stichnot wrote:
> .def

Done.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:21: // MINIMAL build.
On 2016/03/18 19:49:35, stichnot wrote:
> For all the mentions of MINIMAL in this CL, I think it would be more precise
to
> refer to BuildDefs::llvmCl().
> 
> E.g., "MINIMAL build" ==> "non-LLVM_CL build"

Done.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:25: struct release_opt_flag {};
On 2016/03/18 19:49:35, stichnot wrote:
> Instead of these 3 flag types, I wonder about using 3 different X-macro names
> instead?  e.g. X(), Y(), Z()

I though about that, but type tagging is (to me) clearer, and more in C++. Plus,
we would need to define three macros instead of one -- true, they would be
trivial definitions --, which is not standard across the code base.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:32: } // end of namespace Ice
On 2016/03/18 19:49:35, stichnot wrote:
> It's unusual (and therefore slightly jarring to me) to end the Ice namespace
> before the end of the file.

Done.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:35: /* Name, Type, ClType, <<flag declaration ctor
arguments>> */                \
On 2016/03/18 19:49:35, stichnot wrote:
> Suggestion: Pull the cl::init value to the top level (along with Name and
Type)
> to make it easier to generate the non-LLVM_CL const values.  Also, it would
> hopefully simplify the reset/init stuff in IceClFlags.cpp so that no manual
> intervention is needed outside this .def file when adding a new flag.

oh, you spotted my laziness. :)

It turns out that some flags do not have a default -- which is wrong? --, so I
would have to come up with reasonable defaults for them, in this CL.

I decided to go for -- wait for it -- variadic template magic. :)

I added a function in IceClFlags.cpp that "peels" all the non cl::init() stuff
off the __VA_LIST__, until it finds the first cl::init() object, and return
whatever the default is. It is far from perfect -- if there are more than one
cl::init() initializers, the first one is what you get -- and it definitively
does not work with lists. and you will get cryptic error messages if you
initializer is not convertible to the flag type... but the solution is cool,
nonetheless. :)

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:70: \
On 2016/03/18 19:49:35, stichnot wrote:
> How about a big visible separator comment like "Following are flags of
> dev_opt_flag"

Done.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:142: X(ExcludedRegisters, std::string, dev_list_flag,
"reg-exclude",              \
On 2016/03/18 19:49:35, stichnot wrote:
> Move this to the bottom with the rest of the dev_list_flag items?

The flags are now sorted alphabetically. Perhaps we should rename this flag --
in another cl?

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.h#newco...
src/IceClFlags.h:26: #pragma clang diagnostic ignored "-Wredundant-move"
On 2016/03/18 19:49:35, stichnot wrote:
> It looks like we no longer need to ignore redundant-move warnings.  (I guess
the
> other two instances in the code base could also be removed.)

(The other two are) maybe better as part of a clean up CL?

Jim Stichnoth

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def File src/IceClFlags.def (right): https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#newcode35 src/IceClFlags.def:35: /* Name, Type, ClType, <<flag declaration ctor arguments>> */ ...

4 years, 9 months ago (2016-03-21 15:44:47 UTC) #7

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:35: /* Name, Type, ClType, <<flag declaration ctor
arguments>> */                \
On 2016/03/21 14:03:35, John wrote:
> On 2016/03/18 19:49:35, stichnot wrote:
> > Suggestion: Pull the cl::init value to the top level (along with Name and
> Type)
> > to make it easier to generate the non-LLVM_CL const values.  Also, it would
> > hopefully simplify the reset/init stuff in IceClFlags.cpp so that no manual
> > intervention is needed outside this .def file when adding a new flag.
> 
> oh, you spotted my laziness. :)
> 
> It turns out that some flags do not have a default -- which is wrong? --, so I
> would have to come up with reasonable defaults for them, in this CL.
> 
> I decided to go for -- wait for it -- variadic template magic. :)
> 
> I added a function in IceClFlags.cpp that "peels" all the non cl::init() stuff
> off the __VA_LIST__, until it finds the first cl::init() object, and return
> whatever the default is. It is far from perfect -- if there are more than one
> cl::init() initializers, the first one is what you get -- and it definitively
> does not work with lists. and you will get cryptic error messages if you
> initializer is not convertible to the flag type... but the solution is cool,
> nonetheless. :)

This seems HUGELY overly complex, and solely for the purpose of carrying forward
the legacy of someone forgetting to add cl::init(false).

I looked through all the flags in this file, and AFAICT, the omitted inits are
almost entirely for bool flags where cl::init(false) was intended.  The
remainder are for the 3 instances of dev_list_flag, where an empty list is
intended.

I guess there might a problem where the addition of cl::init(DefaultValue) would
need to be suppressed for dev_list_flag flags, but maybe that can be handled
through those *_flag traits?

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.h#newco...
src/IceClFlags.h:26: #pragma clang diagnostic ignored "-Wredundant-move"
On 2016/03/21 14:03:35, John wrote:
> On 2016/03/18 19:49:35, stichnot wrote:
> > It looks like we no longer need to ignore redundant-move warnings.  (I guess
> the
> > other two instances in the code base could also be removed.)
> 
> (The other two are) maybe better as part of a clean up CL?

Either way, given how trivially small the change is / would be.

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.cpp#new...
src/IceClFlags.cpp:34: #include <iostream>
alphabetize includes

Also, any issues with
http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden ?

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.def#new...
src/IceClFlags.def:70: \
tab character...

John

4 years, 9 months ago (2016-03-21 17:06:36 UTC) #8

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.def#new...
src/IceClFlags.def:35: /* Name, Type, ClType, <<flag declaration ctor
arguments>> */                \
On 2016/03/21 15:44:47, stichnot wrote:
> On 2016/03/21 14:03:35, John wrote:
> > On 2016/03/18 19:49:35, stichnot wrote:
> > > Suggestion: Pull the cl::init value to the top level (along with Name and
> > Type)
> > > to make it easier to generate the non-LLVM_CL const values.  Also, it
would
> > > hopefully simplify the reset/init stuff in IceClFlags.cpp so that no
manual
> > > intervention is needed outside this .def file when adding a new flag.
> > 
> > oh, you spotted my laziness. :)
> > 
> > It turns out that some flags do not have a default -- which is wrong? --, so
I
> > would have to come up with reasonable defaults for them, in this CL.
> > 
> > I decided to go for -- wait for it -- variadic template magic. :)
> > 
> > I added a function in IceClFlags.cpp that "peels" all the non cl::init()
stuff
> > off the __VA_LIST__, until it finds the first cl::init() object, and return
> > whatever the default is. It is far from perfect -- if there are more than
one
> > cl::init() initializers, the first one is what you get -- and it
definitively
> > does not work with lists. and you will get cryptic error messages if you
> > initializer is not convertible to the flag type... but the solution is cool,
> > nonetheless. :)
> 
> This seems HUGELY overly complex, and solely for the purpose of carrying
forward
> the legacy of someone forgetting to add cl::init(false).
> 
> I looked through all the flags in this file, and AFAICT, the omitted inits are
> almost entirely for bool flags where cl::init(false) was intended.  The
> remainder are for the 3 instances of dev_list_flag, where an empty list is
> intended.
> 
> I guess there might a problem where the addition of cl::init(DefaultValue)
would
> need to be suppressed for dev_list_flag flags, but maybe that can be handled
> through those *_flag traits?

discussed offline.

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.h
File src/IceClFlags.h (right):

https://codereview.chromium.org/1803403002/diff/100001/src/IceClFlags.h#newco...
src/IceClFlags.h:26: #pragma clang diagnostic ignored "-Wredundant-move"
On 2016/03/21 15:44:47, stichnot wrote:
> On 2016/03/21 14:03:35, John wrote:
> > On 2016/03/18 19:49:35, stichnot wrote:
> > > It looks like we no longer need to ignore redundant-move warnings.  (I
guess
> > the
> > > other two instances in the code base could also be removed.)
> > 
> > (The other two are) maybe better as part of a clean up CL?
> 
> Either way, given how trivially small the change is / would be.

done, but it is good practice (to me... :P) not to mix unrelated changes in the
same patch. For once, the log messages usually fail to omit them. second,
reverting a cl might revert things that should not be reverted. Third, touching
files needlessly increase the likelihood of non-trivial merges.

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.cpp
File src/IceClFlags.cpp (right):

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.cpp#new...
src/IceClFlags.cpp:34: #include <iostream>
On 2016/03/21 15:44:47, stichnot wrote:
> alphabetize includes
> 
> Also, any issues with
> http://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden ?

apart from the fact that the llvm coding style is just dumb? :P

I forgot to remove this... good catch.

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.def
File src/IceClFlags.def (right):

https://codereview.chromium.org/1803403002/diff/140001/src/IceClFlags.def#new...
src/IceClFlags.def:70: \
On 2016/03/21 15:44:47, stichnot wrote:
> tab character...

vim...

John

4 years, 9 months ago (2016-03-21 18:18:07 UTC) #11

Message was sent while issue was closed.

Committed patchset #9 (id:160001) manually as
c5bc5cb33580557062574416794244a09ba99130 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Issue 1803403002: Subzero. Flags refactoring. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Merges ClFlags and ClFlagsExtra #

Patch Set 3 : Command line flags are macro-ized. #

Patch Set 4 : adds IceClFlags.def file #

Patch Set 5 : Add comments. #

Patch Set 6 : git pull #

Patch Set 7 : Addresses comments. #

Patch Set 8 : Remove test code. #

Patch Set 9 : Addresses comments. #

Messages