Initial skeleton of Subzero.

Initial skeleton of Subzero.

This includes just enough code to build the high-level ICE IR and dump it back out again.  There is a script szdiff.py that does a fuzzy diff of the input and output for verification.  See the comment in szdiff.py for a description of the fuzziness.

Building llvm2ice requires LLVM headers, libs, and tools (e.g. FileCheck) to be present.  These default to something like llvm_i686_linux_work/Release+Asserts/ based on the checked-out and built pnacl-llvm code; I'll try to figure out how to more automatically detect the build configuration.

"make check" runs the lit tests.

This CL has under 2000 lines of "interesting" Ice*.{h,cpp} code, plus 600 lines of llvm2ice.cpp driver code, and the rest is tests.

Here is the high-level mapping of source files to functionality:

IceDefs.h, IceTypes.h, IceTypes.cpp:
Commonly used types and utilities.

IceCfg.h, IceCfg.cpp:
Operations at the function level.

IceCfgNode.h, IceCfgNode.cpp:
Operations on basic blocks (nodes).

IceInst.h, IceInst.cpp:
Operations on instructions.

IceOperand.h, IceOperand.cpp:
Operations on operands, such as stack locations, physical registers, and constants.

BUG= none
R=jfb@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f7c9a14

[Mark Seaborn, 2014-03-20 00:09:47]

Some notes about layout and style...

https://codereview.chromium.org/205613002/diff/1/IceCfg.h
File IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode2
IceCfg.h:2: /* Copyright 2014 The Native Client Authors. All rights reserved.
Can you use the LLVM licence and boilerplate style, please?

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode6
IceCfg.h:6: 
Can you put these files in a src/ or lib/ directory to match LLVM and make the
source tree more manageable?

Maybe headers should go in an include/ dir, too?

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode7
IceCfg.h:7: #ifndef _IceCfg_h
Can you use the LLVM style?  i.e. ICECFG_H

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode17
IceCfg.h:17: IceCfg(void);
Can you use "()" (C++ style) rather than "(void)" (C style)?

[Jim Stichnoth, 2014-03-20 22:58:45]

Addressed Mark's initial comments.  Unfortunately, moving the source files into
the src/ subdirectory makes it hard to see the differences between patch sets,
but they are pretty simple:

- (void) ==> ()
- License/description in the file headers
- LLVM-style #include guards (all uppercase)

https://codereview.chromium.org/205613002/diff/1/IceCfg.h
File IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode2
IceCfg.h:2: /* Copyright 2014 The Native Client Authors. All rights reserved.
On 2014/03/20 00:09:47, Mark Seaborn wrote:
> Can you use the LLVM licence and boilerplate style, please?

Done.  (Waiting for a suitable LICENSE.TXT file from the Open Source team.)

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode6
IceCfg.h:6: 
On 2014/03/20 00:09:47, Mark Seaborn wrote:
> Can you put these files in a src/ or lib/ directory to match LLVM and make the
> source tree more manageable?
> 
> Maybe headers should go in an include/ dir, too?

Done.  I moved *.{h,cpp} into the src/ subdirectory.  I changed the Makefile to
put .o files into the build/ subdirectory.

I vastly prefer keeping src and include files in the same directory, compared to
the way LLVM separates them.  Given the relatively small number of Subzero
source files, and the LLVM precedent of mixing src and include files in some
places (e.g. under Target/), I would keep them together.

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode7
IceCfg.h:7: #ifndef _IceCfg_h
On 2014/03/20 00:09:47, Mark Seaborn wrote:
> Can you use the LLVM style?  i.e. ICECFG_H

Done.

https://codereview.chromium.org/205613002/diff/1/IceCfg.h#newcode17
IceCfg.h:17: IceCfg(void);
On 2014/03/20 00:09:47, Mark Seaborn wrote:
> Can you use "()" (C++ style) rather than "(void)" (C style)?

Done.

[JF, 2014-03-21 02:11:30]

https://codereview.chromium.org/205613002/diff/40001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/205613002/diff/40001/.gitignore#newcode3
.gitignore:3:
#==============================================================================#
I'm not sure you really need this.

https://codereview.chromium.org/205613002/diff/40001/.gitignore#newcode30
.gitignore:30: llvm2ice
This builds in-tree?

https://codereview.chromium.org/205613002/diff/40001/Makefile
File Makefile (right):

https://codereview.chromium.org/205613002/diff/40001/Makefile#newcode1
Makefile:1: # The following variables will likely need to be modified, depending
on where
It doesn't matter much now, but I thought we were moving our LLVM-related things
to cmake+ninja in general with the new build scripts. This should build fast for
now, but ninja being the new hotness we should use it.

https://codereview.chromium.org/205613002/diff/40001/README.rst
File README.rst (right):

https://codereview.chromium.org/205613002/diff/40001/README.rst#newcode38
README.rst:38: ``-target=<TARGET>`` -- Set the target architecture (default
x8632).
What are the target architecture that are supported? Everyone writes them
differently...

https://codereview.chromium.org/205613002/diff/40001/szdiff.py
File szdiff.py (right):

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode20
szdiff.py:20: # "LL".
I'd expect this to be a triply-quoted string instead of a comment. Python style.

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode27
szdiff.py:27: argparser = argparse.ArgumentParser()
description='...'

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode28
szdiff.py:28: argparser.add_argument('llfile')
type=argparse.FileType('r') would allow you to read the file straight up.
type=string would at least make it a string.

Also, required=True, metavar='FILE', help='...'

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode29
szdiff.py:29: argparser.add_argument('--llvm2ice')
Same as above, but with default='./llvm2ice' and required=False

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode33
szdiff.py:33: llvm2ice = args.llvm2ice if args.llvm2ice else './llvm2ice'
This can be dropped with the above default=

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode47
szdiff.py:47: stdout=subprocess.PIPE)
You know I sed, but in this case http://docs.python.org/2/howto/regex.html ?

[Jim Stichnoth, 2014-03-21 21:33:08]

https://codereview.chromium.org/205613002/diff/40001/.gitignore
File .gitignore (right):

https://codereview.chromium.org/205613002/diff/40001/.gitignore#newcode3
.gitignore:3:
#==============================================================================#
On 2014/03/21 02:11:30, JF wrote:
> I'm not sure you really need this.

Done.

https://codereview.chromium.org/205613002/diff/40001/.gitignore#newcode30
.gitignore:30: llvm2ice
On 2014/03/21 02:11:30, JF wrote:
> This builds in-tree?

For now, yes.  The real version will abandon the llvm2ice driver.  The rest will
be linked into a subzero library (possibly containing the bitcode reader as
well), and hooked into the PNaCl toolchain through some localmod.

Later I'll work out the specifics of the Makefile with respect to the PNaCl
build system.

https://codereview.chromium.org/205613002/diff/40001/Makefile
File Makefile (right):

https://codereview.chromium.org/205613002/diff/40001/Makefile#newcode1
Makefile:1: # The following variables will likely need to be modified, depending
on where
On 2014/03/21 02:11:30, JF wrote:
> It doesn't matter much now, but I thought we were moving our LLVM-related
things
> to cmake+ninja in general with the new build scripts. This should build fast
for
> now, but ninja being the new hotness we should use it.

Right, more work is needed (likely in a later CL?) to get the details sorted out
with respect to the rest of the PNaCl build environment.

https://codereview.chromium.org/205613002/diff/40001/README.rst
File README.rst (right):

https://codereview.chromium.org/205613002/diff/40001/README.rst#newcode38
README.rst:38: ``-target=<TARGET>`` -- Set the target architecture (default
x8632).
On 2014/03/21 02:11:30, JF wrote:
> What are the target architecture that are supported? Everyone writes them
> differently...

Added to the doc.  For the curious:
- x8632 (default, all optimizations enabled)
- x8632fast (minimal passes, lousy code quality)
- x8664, arm32, arm64 (to be implemented)

https://codereview.chromium.org/205613002/diff/40001/szdiff.py
File szdiff.py (right):

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode20
szdiff.py:20: # "LL".
On 2014/03/21 02:11:30, JF wrote:
> I'd expect this to be a triply-quoted string instead of a comment. Python
style.

Done.

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode27
szdiff.py:27: argparser = argparse.ArgumentParser()
On 2014/03/21 02:11:30, JF wrote:
> description='...'

Done.

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode28
szdiff.py:28: argparser.add_argument('llfile')
On 2014/03/21 02:11:30, JF wrote:
> type=argparse.FileType('r') would allow you to read the file straight up.
> type=string would at least make it a string.

Done.  (BTW 'type=string' is not valid, and the default type is a string.)

> 
> Also, required=True, metavar='FILE', help='...'

Done.  (BTW 'required' is invalid for positional arguments.)

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode29
szdiff.py:29: argparser.add_argument('--llvm2ice')
On 2014/03/21 02:11:30, JF wrote:
> Same as above, but with default='./llvm2ice' and required=False

Done.

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode33
szdiff.py:33: llvm2ice = args.llvm2ice if args.llvm2ice else './llvm2ice'
On 2014/03/21 02:11:30, JF wrote:
> This can be dropped with the above default=

Done.

https://codereview.chromium.org/205613002/diff/40001/szdiff.py#newcode47
szdiff.py:47: stdout=subprocess.PIPE)
On 2014/03/21 02:11:30, JF wrote:
> You know I sed, but in this case http://docs.python.org/2/howto/regex.html ?

OK OK, fixed.

[jvoung (off chromium), 2014-03-22 00:29:38]

quick scan comments

https://codereview.chromium.org/205613002/diff/40001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/205613002/diff/40001/src/IceOperand.h#newcode184
src/IceOperand.h:184: Vars = new IceVariable *[1];
what deletes the Vars?

The IceOperand dtor doesn't and it doesn't look like this uses a scoped array
ptr?

https://codereview.chromium.org/205613002/diff/40001/src/IceOperand.h#newcode196
src/IceOperand.h:196: // DefOrUseNode is the node where this variable was
produced, and is
comment says DefOrUseNode but field name is DefNode

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode120
src/llvm2ice.cpp:120: IceType convertIntegerType(const IntegerType *IntTy) {
is this const?

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode154
src/llvm2ice.cpp:154: return IceType_i32;
This i32 is for function ptr, or function-but-not-function-pointer? If it is a
pointer, perhaps there should be something like intptr_t, so that the code is a
bit more parametric w.r.t pointer size?

https://codereview.chromium.org/205613002/diff/70001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/70001/src/IceCfg.h#newcode15
src/IceCfg.h:15: #ifndef SUBZERO_ICECFG_H
Should the header guards have "_SRC_" now that they sit under subzero/src/... ?

https://codereview.chromium.org/205613002/diff/70001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/70001/src/IceInst.h#newcode60
src/IceInst.h:60: assert(0);
assert(0) ?

[JF, 2014-03-23 00:03:14]

https://codereview.chromium.org/205613002/diff/70001/README.rst
File README.rst (right):

https://codereview.chromium.org/205613002/diff/70001/README.rst#newcode40
README.rst:40: quality) is also available.  Future targets include x8664, arm32,
and arm64.
Bikeshedding names, it's weird to have upper/lower cases mixed, and the names
aren't what we usually go with AFAIK.

I'd expect x86-32, x86-64, arm, mips. ARM likes confusing things, so the new
names in ARMv8 are A32 and A64...

Fast seems orthogonal.

https://codereview.chromium.org/205613002/diff/70001/szdiff.py
File szdiff.py (right):

https://codereview.chromium.org/205613002/diff/70001/szdiff.py#newcode56
szdiff.py:56: line = trailing_space.sub('', line)
Another rstrip would be better.

https://codereview.chromium.org/205613002/diff/70001/szdiff.py#newcode66
szdiff.py:66: 'float [-0-9]', # FP constants
Is that really what FP constants look like? Single digit, or a dash?

[Jim Stichnoth, 2014-03-24 13:18:52]

Comments still to be addressed:

- Better "llvm2ice --target" arg string values (JF). 
https://codereview.chromium.org/205613002/diff/70001/README.rst#newcode40

- High-level pointer type, instead of hard-coding IceType_i32 (Jan). 
https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode154

https://codereview.chromium.org/205613002/diff/40001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/205613002/diff/40001/src/IceOperand.h#newcode184
src/IceOperand.h:184: Vars = new IceVariable *[1];
On 2014/03/22 00:29:39, jvoung (cr) wrote:
> what deletes the Vars?
> 
> The IceOperand dtor doesn't and it doesn't look like this uses a scoped array
> ptr?

Done, good catch.

Most allocation in Subzero is supposed to be done using the BumpPtrAllocator in
IceCfg::Allocator.  This is currently done for instructions, but needs to be
done for operands as well.  I still need to deal with places where STL-style
containers are used, but I hope to defer that for a few more CLs.

https://codereview.chromium.org/205613002/diff/40001/src/IceOperand.h#newcode196
src/IceOperand.h:196: // DefOrUseNode is the node where this variable was
produced, and is
On 2014/03/22 00:29:39, jvoung (cr) wrote:
> comment says DefOrUseNode but field name is DefNode

Done.  (And fixed the same problem in IceDefs.h.)

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode120
src/llvm2ice.cpp:120: IceType convertIntegerType(const IntegerType *IntTy) {
On 2014/03/22 00:29:39, jvoung (cr) wrote:
> is this const?

Done, also convertType() below.

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode154
src/llvm2ice.cpp:154: return IceType_i32;
On 2014/03/22 00:29:39, jvoung (cr) wrote:
> This i32 is for function ptr, or function-but-not-function-pointer? If it is a
> pointer, perhaps there should be something like intptr_t, so that the code is
a
> bit more parametric w.r.t pointer size?

Generally I was following
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#d...
which fixes pointers to i32.  This means using IceType_i32 here should be fine,
but only as long as the target is PNaCl (or the target pointer size is actually
32 bits).

But given long-term goals of Subzero, plus the fact that the initial x86-64
prototype will be native (unsandboxed), you're right and we should have a
high-level pointer type that gets lowered to i32 or i64.  I'll work on that as a
separate patchset.

https://codereview.chromium.org/205613002/diff/70001/README.rst
File README.rst (right):

https://codereview.chromium.org/205613002/diff/70001/README.rst#newcode40
README.rst:40: quality) is also available.  Future targets include x8664, arm32,
and arm64.
On 2014/03/23 00:03:15, JF wrote:
> Bikeshedding names, it's weird to have upper/lower cases mixed, and the names
> aren't what we usually go with AFAIK.
> 
> I'd expect x86-32, x86-64, arm, mips. ARM likes confusing things, so the new
> names in ARMv8 are A32 and A64...

Well, my choices were all lowercase. :)  In any case, I agree that the names
should match PNaCl's (assuming we have those all consistent by now?), or part of
the LLVM target triple, but not something new.

> 
> Fast seems orthogonal.

I wouldn't say orthogonal, but I guess it really refers to the -O level. 
Different -O levels will differ in the list of passes, including code selection
strategy like FastISel versus SelectionDAG.  Maybe we can sort this out after a
couple of architectures are implemented?

https://codereview.chromium.org/205613002/diff/70001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/70001/src/IceCfg.h#newcode15
src/IceCfg.h:15: #ifndef SUBZERO_ICECFG_H
On 2014/03/22 00:29:39, jvoung (cr) wrote:
> Should the header guards have "_SRC_" now that they sit under subzero/src/...
?

Done.

https://codereview.chromium.org/205613002/diff/70001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/70001/src/IceInst.h#newcode60
src/IceInst.h:60: assert(0);
On 2014/03/22 00:29:39, jvoung (cr) wrote:
> assert(0) ?

To catch errors in case some IceCfgNode is constructed without a terminator
instruction at the end.  All the valid ICE terminator instructions override
getTerminatorEdges().

https://codereview.chromium.org/205613002/diff/70001/szdiff.py
File szdiff.py (right):

https://codereview.chromium.org/205613002/diff/70001/szdiff.py#newcode56
szdiff.py:56: line = trailing_space.sub('', line)
On 2014/03/23 00:03:15, JF wrote:
> Another rstrip would be better.

Done.  (I misunderstood what exactly rstrip does.)

https://codereview.chromium.org/205613002/diff/70001/szdiff.py#newcode66
szdiff.py:66: 'float [-0-9]', # FP constants
On 2014/03/23 00:03:15, JF wrote:
> Is that really what FP constants look like? Single digit, or a dash?

We're doing regex golf, right?  The first character after "float " (or "double
") is a digit or dash.

[Karl, 2014-03-24 15:39:06]

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode38
src/IceInst.h:38: Switch,
What about the "unreachable" instruction?

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode352
src/IceInst.h:352: class IceInstLoad : public IceInst {
Load instructions also have alignment.

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode423
src/IceInst.h:423: class IceInstStore : public IceInst {
Store also has alignment.

[Jim Stichnoth, 2014-03-24 15:56:13]

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode38
src/IceInst.h:38: Switch,
On 2014/03/24 15:39:07, Karl wrote:
> What about the "unreachable" instruction?

This isn't listed in
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#l...
.  Should it be?

(Along those lines, I see that Bitcast is listed but I'm not handling that or
testing for it yet.)

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode352
src/IceInst.h:352: class IceInstLoad : public IceInst {
On 2014/03/24 15:39:07, Karl wrote:
> Load instructions also have alignment.

According to
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#l...
, the alignment can be determined by the operand type, so I'm not duplicating it
in the ICE instruction.  Is that reasonable?

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode423
src/IceInst.h:423: class IceInstStore : public IceInst {
On 2014/03/24 15:39:07, Karl wrote:
> Store also has alignment.

(Same comment as for Load alignment.)

[Karl, 2014-03-24 16:07:39]

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode38
src/IceInst.h:38: Switch,
On 2014/03/24 15:56:13, stichnot wrote:
> On 2014/03/24 15:39:07, Karl wrote:
> > What about the "unreachable" instruction?
> 
> This isn't listed in
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#l...
> .  Should it be?
> 
> (Along those lines, I see that Bitcast is listed but I'm not handling that or
> testing for it yet.)

The unreachable instruction appears in finalized bitcode, so I think we need to
handle it.

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode38
src/IceInst.h:38: Switch,
On 2014/03/24 15:56:13, stichnot wrote:
> On 2014/03/24 15:39:07, Karl wrote:
> > What about the "unreachable" instruction?
> 
> This isn't listed in
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#l...
> .  Should it be?
> 
> (Along those lines, I see that Bitcast is listed but I'm not handling that or
> testing for it yet.)

I had assumed that bitcast was one of the "cast" instruction.

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode352
src/IceInst.h:352: class IceInstLoad : public IceInst {
On 2014/03/24 15:56:13, stichnot wrote:
> On 2014/03/24 15:39:07, Karl wrote:
> > Load instructions also have alignment.
> 
> According to
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#l...
> , the alignment can be determined by the operand type, so I'm not duplicating
it
> in the ICE instruction.  Is that reasonable?

You noticed something I missed. I don't know why the type defines alignment the
way it does, but you are right. So, I guess it is alright the way you have it.

[Jim Stichnoth, 2014-03-25 20:01:06]

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/90001/src/IceInst.h#newcode38
src/IceInst.h:38: Switch,
On 2014/03/24 16:07:40, Karl wrote:
> On 2014/03/24 15:56:13, stichnot wrote:
> > On 2014/03/24 15:39:07, Karl wrote:
> > > What about the "unreachable" instruction?
> > 
> > This isn't listed in
> >
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#l...
> > .  Should it be?
> > 
> > (Along those lines, I see that Bitcast is listed but I'm not handling that
or
> > testing for it yet.)
> 
> The unreachable instruction appears in finalized bitcode, so I think we need
to
> handle it.

Done.

[Jim Stichnoth, 2014-03-28 13:52:17]

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode154
src/llvm2ice.cpp:154: return IceType_i32;
On 2014/03/24 13:18:53, stichnot wrote:
> On 2014/03/22 00:29:39, jvoung (cr) wrote:
> > This i32 is for function ptr, or function-but-not-function-pointer? If it is
a
> > pointer, perhaps there should be something like intptr_t, so that the code
is
> a
> > bit more parametric w.r.t pointer size?
> 
> Generally I was following
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#d...
> which fixes pointers to i32.  This means using IceType_i32 here should be
fine,
> but only as long as the target is PNaCl (or the target pointer size is
actually
> 32 bits).
> 
> But given long-term goals of Subzero, plus the fact that the initial x86-64
> prototype will be native (unsandboxed), you're right and we should have a
> high-level pointer type that gets lowered to i32 or i64.  I'll work on that as
a
> separate patchset.

I looked into this some more.  In addition to this particular line of code
(which btw returns the type of a function pointer), the alloca construction also
makes a pointer size decision.  There is also the ptrtoint handling, but as I
understand it, ptrtoint is not really in a proper PNaCl bitcode file, but rather
is generated as part of the bitcode writer to make types consistent.

I think that in these two (or three) cases, it will be best to continue to
pre-lower the pointer types before the initial ICE construction, since we
already know the target, and avoid adding an abstract pointer type to ICE.

[jvoung (off chromium), 2014-03-28 16:17:06]

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode151
src/llvm2ice.cpp:151: return convertType(PTy->getElementType());
Should this also return i32? Otherwise wouldn't it return the pointed-to type
instead of being a straight conversion?

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode154
src/llvm2ice.cpp:154: return IceType_i32;
On 2014/03/28 13:52:18, stichnot wrote:
> On 2014/03/24 13:18:53, stichnot wrote:
> > On 2014/03/22 00:29:39, jvoung (cr) wrote:
> > > This i32 is for function ptr, or function-but-not-function-pointer? If it
is
> a
> > > pointer, perhaps there should be something like intptr_t, so that the code
> is
> > a
> > > bit more parametric w.r.t pointer size?
> > 
> > Generally I was following
> >
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#d...
> > which fixes pointers to i32.  This means using IceType_i32 here should be
> fine,
> > but only as long as the target is PNaCl (or the target pointer size is
> actually
> > 32 bits).
> > 
> > But given long-term goals of Subzero, plus the fact that the initial x86-64
> > prototype will be native (unsandboxed), you're right and we should have a
> > high-level pointer type that gets lowered to i32 or i64.  I'll work on that
as
> a
> > separate patchset.
> 
> I looked into this some more.  In addition to this particular line of code
> (which btw returns the type of a function pointer), the alloca construction
also
> makes a pointer size decision.  There is also the ptrtoint handling, but as I
> understand it, ptrtoint is not really in a proper PNaCl bitcode file, but
rather
> is generated as part of the bitcode writer to make types consistent.
> 
> I think that in these two (or three) cases, it will be best to continue to
> pre-lower the pointer types before the initial ICE construction, since we
> already know the target, and avoid adding an abstract pointer type to ICE.


Okay seems fine to keep this the way it is then. I wasn't actually suggesting to
add IceType_PtrTy in the enums, but something more like LLVM's
Subtarget->getPointerTy() which returns MVT::i32 (which would seem like
IceType_i32).

[Jim Stichnoth, 2014-03-28 16:45:07]

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode154
src/llvm2ice.cpp:154: return IceType_i32;
On 2014/03/28 16:17:07, jvoung (cr) wrote:
> On 2014/03/28 13:52:18, stichnot wrote:
> > On 2014/03/24 13:18:53, stichnot wrote:
> > > On 2014/03/22 00:29:39, jvoung (cr) wrote:
> > > > This i32 is for function ptr, or function-but-not-function-pointer? If
it
> is
> > a
> > > > pointer, perhaps there should be something like intptr_t, so that the
code
> > is
> > > a
> > > > bit more parametric w.r.t pointer size?
> > > 
> > > Generally I was following
> > >
> >
>
https://developers.google.com/native-client/dev/reference/pnacl-bitcode-abi#d...
> > > which fixes pointers to i32.  This means using IceType_i32 here should be
> > fine,
> > > but only as long as the target is PNaCl (or the target pointer size is
> > actually
> > > 32 bits).
> > > 
> > > But given long-term goals of Subzero, plus the fact that the initial
x86-64
> > > prototype will be native (unsandboxed), you're right and we should have a
> > > high-level pointer type that gets lowered to i32 or i64.  I'll work on
that
> as
> > a
> > > separate patchset.
> > 
> > I looked into this some more.  In addition to this particular line of code
> > (which btw returns the type of a function pointer), the alloca construction
> also
> > makes a pointer size decision.  There is also the ptrtoint handling, but as
I
> > understand it, ptrtoint is not really in a proper PNaCl bitcode file, but
> rather
> > is generated as part of the bitcode writer to make types consistent.
> > 
> > I think that in these two (or three) cases, it will be best to continue to
> > pre-lower the pointer types before the initial ICE construction, since we
> > already know the target, and avoid adding an abstract pointer type to ICE.
> 
> 
> Okay seems fine to keep this the way it is then. I wasn't actually suggesting
to
> add IceType_PtrTy in the enums, but something more like LLVM's
> Subtarget->getPointerTy() which returns MVT::i32 (which would seem like
> IceType_i32).

Done.

[jvoung (off chromium), 2014-03-28 23:21:53]

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode151
src/llvm2ice.cpp:151: return convertType(PTy->getElementType());
On 2014/03/28 16:17:07, jvoung (cr) wrote:
> Should this also return i32? Otherwise wouldn't it return the pointed-to type
> instead of being a straight conversion?

ping?

https://codereview.chromium.org/205613002/diff/170001/Makefile
File Makefile (right):

https://codereview.chromium.org/205613002/diff/170001/Makefile#newcode31
Makefile:31: CXXFLAGS := -Wall -Werror -fno-rtti $(OPTLEVEL) -g $(LLVM_CXXFLAGS)
-m32
-fno-exceptions too?

https://codereview.chromium.org/205613002/diff/170001/README.rst
File README.rst (right):

https://codereview.chromium.org/205613002/diff/170001/README.rst#newcode11
README.rst:11: Set variables ``LLVM_SRC_PATH``, ``LLVM_BUILD_PATH``, and
``LLVM_BIN_PATH`` to
I assume LLVM_BUILD_PATH will come in later (seems unused by the Makefile right
now)

https://codereview.chromium.org/205613002/diff/170001/src/IceCfgNode.h
File src/IceCfgNode.h (right):

https://codereview.chromium.org/205613002/diff/170001/src/IceCfgNode.h#newcode25
src/IceCfgNode.h:25: return new IceCfgNode(Cfg, LabelIndex, Name);
should this new() use the cfg allocator too?

[Jim Stichnoth, 2014-03-29 14:23:22]

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/205613002/diff/40001/src/llvm2ice.cpp#newcode151
src/llvm2ice.cpp:151: return convertType(PTy->getElementType());
On 2014/03/28 23:21:53, jvoung (cr) wrote:
> On 2014/03/28 16:17:07, jvoung (cr) wrote:
> > Should this also return i32? Otherwise wouldn't it return the pointed-to
type
> > instead of being a straight conversion?
> 
> ping?

Sorry, done.  This was left over from earlier representation/lowering of the
Load and Store instructions, where I wanted to capture the pointed-to type of
the address operand.  It worked because Load and Store instructions are
basically the only places where we find pointers in the simplified PNaCl
bitcode.  However, we actually get the pointed-to type of the address operand by
looking at the data operand in the instruction, so this conversion is
unnecessary.

(Making this simplification exposed a bug in the constant pooling code, where
the immediate offset wasn't being included in the lookup key.  This caused the
two halves of an i64 global to be collapsed to the same address during
lowering.)

https://codereview.chromium.org/205613002/diff/170001/Makefile
File Makefile (right):

https://codereview.chromium.org/205613002/diff/170001/Makefile#newcode31
Makefile:31: CXXFLAGS := -Wall -Werror -fno-rtti $(OPTLEVEL) -g $(LLVM_CXXFLAGS)
-m32
On 2014/03/28 23:21:53, jvoung (cr) wrote:
> -fno-exceptions too?

Done.

https://codereview.chromium.org/205613002/diff/170001/README.rst
File README.rst (right):

https://codereview.chromium.org/205613002/diff/170001/README.rst#newcode11
README.rst:11: Set variables ``LLVM_SRC_PATH``, ``LLVM_BUILD_PATH``, and
``LLVM_BIN_PATH`` to
On 2014/03/28 23:21:53, jvoung (cr) wrote:
> I assume LLVM_BUILD_PATH will come in later (seems unused by the Makefile
right
> now)

Done.  Actually LLVM_BUILD_PATH was removed from the Makefile but I forgot the
README.rst. :)

https://codereview.chromium.org/205613002/diff/170001/src/IceCfgNode.h
File src/IceCfgNode.h (right):

https://codereview.chromium.org/205613002/diff/170001/src/IceCfgNode.h#newcode25
src/IceCfgNode.h:25: return new IceCfgNode(Cfg, LabelIndex, Name);
On 2014/03/28 23:21:53, jvoung (cr) wrote:
> should this new() use the cfg allocator too?

Done, also in IceVariable.  I'm leaving the IceConstants alone for now since
they may live longer than the Cfg in a global constant pool.

[JF, 2014-04-04 04:05:25]

A few comments, more to come :)

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.cpp#newcode61
src/IceCfg.cpp:61: NextInstNumber(1) {
Missing Allocator, Nodes, Variables, Args.

Can ConstantPool also be in the initializer list?

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode31
src/IceCfg.h:31: void setReturnType(IceType ReturnType) { Type = ReturnType; }
The return type of what? The function associated with the CFG?

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode103
src/IceCfg.h:103: mutable IceOstream Str;
This would be nicer in a separate context object or something passed in to
IceCfg at construction time.

Also, mutable, ew.

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode105
src/IceCfg.h:105: private:
Disable copy/assign:
 
http://llvm.org/docs/CodingStandards.html#use-llvm-deleted-function-to-mark-u...

Unless you actually want them, in which case they should be explicit.

Same for other classes.

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode124
src/IceCfg.h:124: class IceConstantPool *ConstantPool;
Use a smart pointer here.

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.cpp#newcode35
src/IceTypes.cpp:35: break;
This would be a great use of xmacros. Put the enum in its own file, and have one
column for the size, one for the string (for the stream below).

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h#newcode28
src/IceTypes.h:28: IceType_NUM
Comment on IceType_NUM needing to be the last enum.

Also see comment on xmacros in the implementation file.

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h#newcode31
src/IceTypes.h:31: uint32_t iceTypeWidth(IceType T);
Bits or bytes? I'd rather have all width related functions say what their unit
is.

It should also return a size_t.

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h#newcode32
src/IceTypes.h:32: IceOstream &operator<<(class IceOstream &Str, IceType T);
Could you make this a template so it works with any stream type? The
implementation doesn't look like it has anything special going on with
IceOstream.

The implementation will need to be moved to the header.

[Jim Stichnoth, 2014-04-06 02:16:08]

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.cpp#newcode61
src/IceCfg.cpp:61: NextInstNumber(1) {
On 2014/04/04 04:05:26, JF wrote:
> Missing Allocator, Nodes, Variables, Args.
> 
> Can ConstantPool also be in the initializer list?

ConstantPool added.

It seems that Allocator can't be added to the initializer list because its copy
constructor is private (and "deleted"), so it gives a compiler error even if the
copy operation could ultimately be elided.

As for Nodes, Variables, and Args, they were intentionally omitted from the
initializer list because they are simple containers and I want them initialized
to the default empty container.  LLVM code appears to regularly omit explicit
initializers in these cases.

As such, I'd prefer to leave these fields off the explicit initializer list.

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode31
src/IceCfg.h:31: void setReturnType(IceType ReturnType) { Type = ReturnType; }
On 2014/04/04 04:05:26, JF wrote:
> The return type of what? The function associated with the CFG?

Yes, this is in a comment 3 lines above. :)

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode103
src/IceCfg.h:103: mutable IceOstream Str;
On 2014/04/04 04:05:26, JF wrote:
> This would be nicer in a separate context object or something passed in to
> IceCfg at construction time.
> 
> Also, mutable, ew.

If you don't mind, I'd love to defer this cleanup after the first round of CLs,
to include:
* Separate ostreams for dumping versus code emission
* Separate allocators for per-function versus global (e.g. global constant pool)
* Other issues that I haven't yet dealt with such as global initializers

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode105
src/IceCfg.h:105: private:
On 2014/04/04 04:05:26, JF wrote:
> Disable copy/assign:
>  
>
http://llvm.org/docs/CodingStandards.html#use-llvm-deleted-function-to-mark-u...
> 
> Unless you actually want them, in which case they should be explicit.
> 
> Same for other classes.

Done.

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode124
src/IceCfg.h:124: class IceConstantPool *ConstantPool;
On 2014/04/04 04:05:26, JF wrote:
> Use a smart pointer here.

Done.

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.cpp#newcode35
src/IceTypes.cpp:35: break;
On 2014/04/04 04:05:26, JF wrote:
> This would be a great use of xmacros. Put the enum in its own file, and have
one
> column for the size, one for the string (for the stream below).

Done, but using tables instead for now.  (I can change to macros if the need
arises.)

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h#newcode28
src/IceTypes.h:28: IceType_NUM
On 2014/04/04 04:05:26, JF wrote:
> Comment on IceType_NUM needing to be the last enum.
> 
> Also see comment on xmacros in the implementation file.

Done.  IceType_NUM is no longer used in this CL, but I'll keep it for now since
the next CL uses it to size an array that maps types to register classes.

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h#newcode31
src/IceTypes.h:31: uint32_t iceTypeWidth(IceType T);
On 2014/04/04 04:05:26, JF wrote:
> Bits or bytes? I'd rather have all width related functions say what their unit
> is.
> 
> It should also return a size_t.

Done.

https://codereview.chromium.org/205613002/diff/190001/src/IceTypes.h#newcode32
src/IceTypes.h:32: IceOstream &operator<<(class IceOstream &Str, IceType T);
On 2014/04/04 04:05:26, JF wrote:
> Could you make this a template so it works with any stream type? The
> implementation doesn't look like it has anything special going on with
> IceOstream.
> 
> The implementation will need to be moved to the header.

I made a templated operator << override which makes things much cleaner.  But I
suspect I didn't address your specific concern about this line?

[Jim Stichnoth, 2014-04-09 05:34:26]

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/190001/src/IceCfg.h#newcode103
src/IceCfg.h:103: mutable IceOstream Str;
On 2014/04/06 02:16:09, stichnot wrote:
> On 2014/04/04 04:05:26, JF wrote:
> > This would be nicer in a separate context object or something passed in to
> > IceCfg at construction time.
> > 
> > Also, mutable, ew.
> 
> If you don't mind, I'd love to defer this cleanup after the first round of
CLs,
> to include:
> * Separate ostreams for dumping versus code emission
> * Separate allocators for per-function versus global (e.g. global constant
pool)
> * Other issues that I haven't yet dealt with such as global initializers

Never mind the deferral request.  The latest patchset provides this global
context, which includes the output streams.  It also includes a global allocator
from which constants are allocated and pooled.

[JF, 2014-04-16 01:27:31]

I went through all of the .h/.cpp files, but not through the tests yet.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode54
src/IceCfg.h:54: IceCfgNode *makeNode(const IceString &Name = "");
Your CFG doesn't seem to have an exit node? How do you determine exit, by doing
RPO and traversing?

I get that any block could have a return instruction, but it seems useful for
some algorithms to have an exit node with edges from each of those node.

Or maybe that's just what I'm used to.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode71
src/IceCfg.h:71: void registerEdges();
What does this do?

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode73
src/IceCfg.h:73: const IceCfgNode *getCurrentNode() const { return CurrentNode;
}
What are current nodes in a CFG?

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode74
src/IceCfg.h:74: void dump();
Your other dump functions seem to take in a stream to dump to.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode98
src/IceCfg.h:98: bool IsInternal; // internal linkage
The comments above could just be the variable names instead.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode99
src/IceCfg.h:99: bool HasError;
Isn't that just ErrorMessage.size() != 0 ?

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:29: sprintf(buf, "__%u", getIndex());
snprintf

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.h
File src/IceCfgNode.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.h#newcode37
src/IceCfgNode.h:37: void registerEdges();
What does this do?

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode21
src/IceDefs.h:21: #include <stdio.h> // sprintf
Since this is a new code base I'd go with the <c*> headers instead of <*.h>
headers for C things:
  http://www.gotw.ca/publications/migrating_to_namespaces.htm

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode34
src/IceDefs.h:34: 
It would be nicer to put everything in a namespace. All the Ice prefixes could
be dropped, and it's the cleaner thing to do anyways.

I'm not sure what cool namespace name you could use. Ice is the obvious one. SZ
isn't a great idea since people use that often.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode59
src/IceDefs.h:59: IceValueTranslation() {}
dtor, copy ctor, assign?

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode89
src/IceDefs.h:89: typedef uint32_t IceVerboseMask;
Why typedef this to uint32_t instead of just using the enum?

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode99
src/IceDefs.h:99: llvm::raw_ostream *Stream;
dtor, copy ctor, assign.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode109
src/IceDefs.h:109: class IceTimer {
Can you put a TODO to use std::chrono when we sync to LLVM 3.5 and get C++11?

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:22: // TODO: Try to refactor the getOrAdd*() methods.
Yes please!

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:29: KeyTypeReloc(Type, std::pair<IceString,
int64_t>(Name, Offset)));
std::make_pair

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:77: // KeyTypeReloc is a triple of {Type, Name,
Offset}.
TODO: use tuple when we move to C++11.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:89: std::vector<IceConstantDouble *> DoublePool;
I find the map/vector duality really weird: the map is from the value you want
to have in the constant pool to the index number at which it was added, and the
vector then has one entry per index.

Why not just use the map?

It's also weird to use float and double as keys in a map, you're going to have a
bad time e.g. with NaN. You want to make sure you preserve all bits of
information provided by the program, but I assume you also want to do some
partial evaluation / constant propagation at some point?

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:115: if (strlen(Buffer) < BaseLength)
Could you explain what this case is?

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:120: sprintf(NewNumber, "_Z%u%s%s", BaseLength,
getTestPrefix().c_str(), Buffer);
snprintf, and check for errors.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:145: Const->setSuppressMangling(SuppressMangling);
Doesn't this change the manging if e.g. the symbol added originally had
mangling, and the second time around it didn't?

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:60: // reserved to hold something LLVM-specific to
facilitate linking.
What would handle hold?

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:69: IceOstream StrEmit; // Stream for code emission
Public?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode25
src/IceInst.cpp:25: Srcs = Cfg->allocateArrayOf<IceOperand *>(MaxSrcs);
Move to the initialization list.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode54
src/IceInst.cpp:54: case Fmul:
Fadd and Fmul aren't commutative.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode59
src/IceInst.cpp:59: default:
It would be better to list everything so that if you add a new type of
arithmetic operation the compiler sees that you don't handle all cases and gives
you a warning.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode79
src/IceInst.cpp:79: addSource(Source);
It doesn't look like you have an invariant on True==False w.r.t. Source's value.
What happens if a users does getSrc(0) on an unconditional branch?

Would it be cleaner if conditional and unconditional were separate instructions?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode298
src/IceInst.cpp:298: break;
Another place where xmacros would be nice ;-)

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode386
src/IceInst.cpp:386: break;
xmacros :-)

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode427
src/IceInst.cpp:427: break;
Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode486
src/IceInst.cpp:486: break;
Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode39
src/IceInst.h:39: Unreachable
I'd put Unreachable as InstKind 0.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode61
src/IceInst.h:61: assert(0);
?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode76
src/IceInst.h:76: virtual ~IceInst() {}
Srcs doesn't get deleted?

You should also probably put the virtual dtors as private in each derived class.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode94
src/IceInst.h:94: bool HasSideEffects;
Having this in every Inst seems silly when it's a property of the instruction
kind. I'd put it in a table indexed by InstKind... Using you guessed it xmacros!

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode97
src/IceInst.h:97: const uint32_t MaxSrcs; // only used for assert
size_t

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode99
src/IceInst.h:99: IceOperand **Srcs;
The ideal here would be to use C's zero-sized arrays since it reduces
indirection and makes for one less allocation, but that wouldn't work because of
inheritance.

SmallVector would be good instead, since most instruction types only use 1 or 2
sources.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode106
src/IceInst.h:106: template <> IceOstream &operator<<(IceOstream &Str, const
IceInst &I);
I think you want this template specialization in IceDefs.h, otherwise you'll get
an ODR violation if IceInst.h ins't also included but the operator is used with
IceInst (unlikely since it requires a const IceInst & to do so).

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode109
src/IceInst.h:109: // and the alignment.
Are there any special meanings for alignment? What does 0 mean?

It would be nice to have a higher level list of which src means what for each of
the instruction types (instead of having to hunt for each instruction type, it's
cleaner in one location). xmacros for this too?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode119
src/IceInst.h:119: static bool classof(const IceInst *Inst) { return
Inst->getKind() == Alloca; }
This could also be auto-defined (albeit not declared) through xmacros.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode134
src/IceInst.h:134: // Ordered by
http://llvm.org/docs/LangRef.html#binary-operations
Why does the order matter? Is the goal to match the order in the NaCl
BitCodeReader? If so the order isn't guaranteed to be the same.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode158
src/IceInst.h:158: IceInstArithmetic(Cfg, Op, Dest, Source1, Source2);
Do the operand types necessarily match?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode199
src/IceInst.h:199: // unconditional branches.
The implementation says that True==False means unconditional. The API should
explain this.

Also, getSrc(0) is the condition?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode239
src/IceInst.h:239: IceOperand *CallTarget, bool Tail) {
Explain Tail: is it actually tailcall, or LLVM's weird idea of tailcall?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode270
src/IceInst.h:270: // Ordered by
http://llvm.org/docs/LangRef.html#conversion-operations
Ditto on ordering.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode296
src/IceInst.h:296: OpKind CastKind;
const, to keep with the other classes.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode304
src/IceInst.h:304: // Ordered by http://llvm.org/docs/LangRef.html#id254
Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode325
src/IceInst.h:325: IceInstFcmp(Cfg, Condition, Dest, Source1, Source2);
Do the operand types necessarily match?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode336
src/IceInst.h:336: FCond Condition;
const, to keep with the other classes.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode344
src/IceInst.h:344: // Ordered by http://llvm.org/docs/LangRef.html#id249
Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode359
src/IceInst.h:359: IceInstIcmp(Cfg, Condition, Dest, Source1, Source2);
Do the operand types necessarily match?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode370
src/IceInst.h:370: ICond Condition;
const, to keep with the other classes.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode373
src/IceInst.h:373: // Load instruction.  The source address is captured in
getSrc(0);
Could you infer alignment? We guarantee that FP is naturally aligned, and we
know the alignment of globals, so you could get better perf out of that
information.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode408
src/IceInst.h:408: IceCfgNode **Labels;
dtor that deletes Labels.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode413
src/IceInst.h:413: // getSrcSize()==0.
Create an isVoid method that does this, so code relying on this pattern is
easier to read.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode416
src/IceInst.h:416: static IceInstRet *create(IceCfg *Cfg, IceOperand *Source =
NULL) {
Source is kind of a bad name here (and in the implementation) since it's the
returned value.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode436
src/IceInst.h:436: IceInstSelect(Cfg, Dest, Condition, SourceTrue, SourceFalse);
Do the operand's type necessarily match?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode499
src/IceInst.h:499: IceCfgNode *LabelDefault;
Why not put default at the start of Labels?

It would simplify getTerminatorEdges.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode502
src/IceInst.h:502: IceCfgNode **Labels; // size is NumCases
dtor that deletes Values and Labels.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:55: sprintf(buf, "__%u", getIndex());
snprintf

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode12
src/IceOperand.h:12: // LLVM variable that is either register- or
stack-allocated, and the
Do variables even have a concept of stack at the IR level? Or are they just SSA
values?

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode27
src/IceOperand.h:27: Constant,
What's constant but not Int/FP/Reloc?

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode50
src/IceOperand.h:50: virtual void setUse(const IceInst *Inst, const IceCfgNode
*Node) {}
Make this pure virtual, error out for constant and keep variable as-is?

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode53
src/IceOperand.h:53: virtual ~IceOperand() {}
Ditto on private virtual dtors in derived classes.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode56
src/IceOperand.h:56: IceOperand(OperandKind Kind, IceType Type) : Type(Type),
Kind(Kind) {}
You should initialize NumVars(0), Vars(NULL) to be safe, or add them as
arguments to the ctor.

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.cpp#newcode30
src/IceTypes.cpp:30: { IceType_f64, 8, "double" }, };
I would still use xmacros here, since it prevents this from going out of sync
with the enum in the .h, and it's pretty regular. That would also make
iceTypeWidthInBytes simpler.

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.cpp#newcode43
src/IceTypes.cpp:43: assert(Width && "Invalid type for iceTypeWidthInBytes()");
Is it expected that IceType_void asserts here?

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.h#newcode33
src/IceTypes.h:33: template <> IceOstream &operator<<(class IceOstream &Str,
const IceType &Type);
Same on this being in IceDefs.h

[Jim Stichnoth, 2014-04-21 20:26:39]

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode54
src/IceCfg.h:54: IceCfgNode *makeNode(const IceString &Name = "");
On 2014/04/16 01:27:32, JF wrote:
> Your CFG doesn't seem to have an exit node? How do you determine exit, by
doing
> RPO and traversing?
> 
> I get that any block could have a return instruction, but it seems useful for
> some algorithms to have an exit node with edges from each of those node.
> 
> Or maybe that's just what I'm used to.

So far, the only Subzero pass/algorithm that iterates over nodes in non
arbitrary order is liveness analysis.  However, liveness analysis doesn't need
to know about exit nodes.  It just has to iterate over the nodes until
convergence, and convergence may happen sooner iteration happens in reverse
topological order (and the upstream LLVM tools happen to produce a topological
sort).

In any case, the next CL will have a bool IceCfgNode::HasReturn flag in each
node, to help quickly find nodes that need epilog insertion.  This could be
generalized to all exit nodes (not just those that need epilogs) and collected
into IceCfg, but I'd rather hold off until a concrete need arises.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode71
src/IceCfg.h:71: void registerEdges();
On 2014/04/16 01:27:32, JF wrote:
> What does this do?

This was documented in IceCfgNode::registerEdges(), but yeah.

I changed the name registerEdges --> computePredecessors and added a bit of
documentation here.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode73
src/IceCfg.h:73: const IceCfgNode *getCurrentNode() const { return CurrentNode;
}
On 2014/04/16 01:27:32, JF wrote:
> What are current nodes in a CFG?

This is described in the documentation below of the CurrentNode field.  I added
a bit of text here as well.

However, I'm having second thoughts about this whole approach of incrementally
computing whether variables are entirely block-local, versus "global".  It might
be better, less fragile, to construct this information in a separate pass.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode74
src/IceCfg.h:74: void dump();
On 2014/04/16 01:27:32, JF wrote:
> Your other dump functions seem to take in a stream to dump to.

Are you looking at the latest patchset?  The dump() routines are now taking an
IceCfg* argument, which seems unnecessary for IceCfg::dump().

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode98
src/IceCfg.h:98: bool IsInternal; // internal linkage
On 2014/04/16 01:27:32, JF wrote:
> The comments above could just be the variable names instead.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode99
src/IceCfg.h:99: bool HasError;
On 2014/04/16 01:27:32, JF wrote:
> Isn't that just ErrorMessage.size() != 0 ?

Not if you call setError("blah") followed by setError("").  Once an error,
always an error.

There's no strong need to save a word of space at the CFG level, so I'd prefer
not to use sentinel values here.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:29: sprintf(buf, "__%u", getIndex());
On 2014/04/16 01:27:32, JF wrote:
> snprintf

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.h
File src/IceCfgNode.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfgNode.h#newcode37
src/IceCfgNode.h:37: void registerEdges();
On 2014/04/16 01:27:32, JF wrote:
> What does this do?

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode21
src/IceDefs.h:21: #include <stdio.h> // sprintf
On 2014/04/16 01:27:32, JF wrote:
> Since this is a new code base I'd go with the <c*> headers instead of <*.h>
> headers for C things:
>   http://www.gotw.ca/publications/migrating_to_namespaces.htm

Done for <cassert> and <cstdio>.  stdint.h seems resistant to this change.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode34
src/IceDefs.h:34: 
On 2014/04/16 01:27:32, JF wrote:
> It would be nicer to put everything in a namespace. All the Ice prefixes could
> be dropped, and it's the cleaner thing to do anyways.
> 
> I'm not sure what cool namespace name you could use. Ice is the obvious one.
SZ
> isn't a great idea since people use that often.

Done.  Used the "Ice" namespace.  In some cases, I deliberately kept the Ice
prefix as well, including IceTimer, IceVerbose, IceCfg.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode59
src/IceDefs.h:59: IceValueTranslation() {}
On 2014/04/16 01:27:32, JF wrote:
> dtor, copy ctor, assign?

Done.  No dtor for this simple wrapper class.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode89
src/IceDefs.h:89: typedef uint32_t IceVerboseMask;
On 2014/04/16 01:27:32, JF wrote:
> Why typedef this to uint32_t instead of just using the enum?

IceVerboseMask is a bitmask of IceVerbose values.  If I "typedef IceVerbose
IceVerboseMask", I get warnings/errors from IceGlobalContext.h and llvm2ice.cpp.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode99
src/IceDefs.h:99: llvm::raw_ostream *Stream;
On 2014/04/16 01:27:32, JF wrote:
> dtor, copy ctor, assign.

Done.  No dtor for this simple wrapper class.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode109
src/IceDefs.h:109: class IceTimer {
On 2014/04/16 01:27:32, JF wrote:
> Can you put a TODO to use std::chrono when we sync to LLVM 3.5 and get C++11?

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:22: // TODO: Try to refactor the getOrAdd*() methods.
On 2014/04/16 01:27:32, JF wrote:
> Yes please!

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:29: KeyTypeReloc(Type, std::pair<IceString,
int64_t>(Name, Offset)));
On 2014/04/16 01:27:32, JF wrote:
> std::make_pair

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:77: // KeyTypeReloc is a triple of {Type, Name,
Offset}.
On 2014/04/16 01:27:32, JF wrote:
> TODO: use tuple when we move to C++11.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:89: std::vector<IceConstantDouble *> DoublePool;
On 2014/04/16 01:27:32, JF wrote:
> I find the map/vector duality really weird: the map is from the value you want
> to have in the constant pool to the index number at which it was added, and
the
> vector then has one entry per index.
> 
> Why not just use the map?

The latest patchset makes this map/vector duality less obvious, but the issue is
still there.  What's happening is that the map holds the tuple that is used to
construct the the IceConstant object, and the vector holds the actual
IceConstant object.

> 
> It's also weird to use float and double as keys in a map, you're going to have
a
> bad time e.g. with NaN. You want to make sure you preserve all bits of
> information provided by the program, but I assume you also want to do some
> partial evaluation / constant propagation at some point?

In the updated code, memcmp is used for the comparison.  There is a TODO to
allow a custom comparator for when the tuple contains fields like non-pooled
strings.

BTW, think the consequence of using FP keys with ordinary comparisons would be
that no matches would be found and you'd end up with duplicates in the pool.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:115: if (strlen(Buffer) < BaseLength)
On 2014/04/16 01:27:32, JF wrote:
> Could you explain what this case is?

Done.  I actually rolled in a better version of mangleName(), which deals with
nesting/namespaces as referenced in the TODO above.  The parsing is better
documented.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:120: sprintf(NewNumber, "_Z%u%s%s", BaseLength,
getTestPrefix().c_str(), Buffer);
On 2014/04/16 01:27:32, JF wrote:
> snprintf, and check for errors.

Done.  I don't think error (or snprintf return value) checking is necessary
here, since if somehow the output buffer space was miscalculated, snprintf will
DTRT and produce a consistent result for mangleName() calls on the same input
string.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:145: Const->setSuppressMangling(SuppressMangling);
On 2014/04/16 01:27:32, JF wrote:
> Doesn't this change the manging if e.g. the symbol added originally had
> mangling, and the second time around it didn't?

True.  The code has been changed to add SuppressMangling to the comparison key.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:60: // reserved to hold something LLVM-specific to
facilitate linking.
On 2014/04/16 01:27:32, JF wrote:
> What would handle hold?

I can't really say concretely, so I killed this whole Handle idea.  We can bring
it back in the future if it turns out to be useful.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:69: IceOstream StrEmit; // Stream for code emission
On 2014/04/16 01:27:32, JF wrote:
> Public?

Make them private, with public accessor methods.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode25
src/IceInst.cpp:25: Srcs = Cfg->allocateArrayOf<IceOperand *>(MaxSrcs);
On 2014/04/16 01:27:32, JF wrote:
> Move to the initialization list.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode54
src/IceInst.cpp:54: case Fmul:
On 2014/04/16 01:27:32, JF wrote:
> Fadd and Fmul aren't commutative.

Done.  I was sad to learn this.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode59
src/IceInst.cpp:59: default:
On 2014/04/16 01:27:32, JF wrote:
> It would be better to list everything so that if you add a new type of
> arithmetic operation the compiler sees that you don't handle all cases and
gives
> you a warning.

Done.  And converted to x-macros.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode79
src/IceInst.cpp:79: addSource(Source);
On 2014/04/16 01:27:32, JF wrote:
> It doesn't look like you have an invariant on True==False w.r.t. Source's
value.
> What happens if a users does getSrc(0) on an unconditional branch?

You crash, of course. :)  Here and in all other instructions, the user is
supposed to make sure their src/dest accesses are valid.  For the branch
instruction, that means testing for !isUnconditional() before accessing the
source operand.  I think the right thing to do here is to add a getCondition()
accessor that at a minimum gives a slightly more informative assert() when
called incorrectly.

> 
> Would it be cleaner if conditional and unconditional were separate
instructions?

I'd rather keep it combined because: (1) LLVM has a single branch instruction so
there's precedent, familiarity, etc.; (2) for the lowered branch instruction,
there are actually 4 variants - conditional versus unconditional, and target
being an intra-block instruction versus the more usual target being the first
instruction of a basic block - so I'd rather not get too deep into that mess.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode298
src/IceInst.cpp:298: break;
On 2014/04/16 01:27:32, JF wrote:
> Another place where xmacros would be nice ;-)

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode386
src/IceInst.cpp:386: break;
On 2014/04/16 01:27:32, JF wrote:
> xmacros :-)

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode427
src/IceInst.cpp:427: break;
On 2014/04/16 01:27:32, JF wrote:
> Ditto.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode486
src/IceInst.cpp:486: break;
On 2014/04/16 01:27:32, JF wrote:
> Ditto.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode39
src/IceInst.h:39: Unreachable
On 2014/04/16 01:27:32, JF wrote:
> I'd put Unreachable as InstKind 0.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode61
src/IceInst.h:61: assert(0);
On 2014/04/16 01:27:32, JF wrote:
> ?

Jan had the same comment. :)  Added a comment to explain that legitimate
terminator instructions override this method with a non-asserting
implementation.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode76
src/IceInst.h:76: virtual ~IceInst() {}
On 2014/04/16 01:27:32, JF wrote:
> Srcs doesn't get deleted?
> 
> You should also probably put the virtual dtors as private in each derived
class.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode94
src/IceInst.h:94: bool HasSideEffects;
On 2014/04/16 01:27:32, JF wrote:
> Having this in every Inst seems silly when it's a property of the instruction
> kind. I'd put it in a table indexed by InstKind... Using you guessed it
xmacros!

It's not purely computable from the instruction kind.  It's true that right now,
only Call instructions are marked as having side effects.  But I expect to be
able to mark certain (most?) intrinsic calls as lacking side effects and subject
to dead-code elimination.  I can think of other distinctions as well in non-Call
instructions.  Because of this, I'd prefer to keep this in the IceInst for now. 
Likely making it a 1-bit field when we get to the tuning phase.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode97
src/IceInst.h:97: const uint32_t MaxSrcs; // only used for assert
On 2014/04/16 01:27:32, JF wrote:
> size_t

Changed this and many other instances of uint32_t to IceSize_t.

For now, I'm keeping IceSize_t at 32 bits.  It could probably be shrunk to 8 or
16 bits, but Call, Switch, and Phi instructions could potentially have more
operands than a uint16_t can represent.  (Keeping in mind that the source
language may not be C or C++.)

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode99
src/IceInst.h:99: IceOperand **Srcs;
On 2014/04/16 01:27:32, JF wrote:
> The ideal here would be to use C's zero-sized arrays since it reduces
> indirection and makes for one less allocation, but that wouldn't work because
of
> inheritance.
> 
> SmallVector would be good instead, since most instruction types only use 1 or
2
> sources.

I don't want IceInst or IceOperand to hold any containers like SmallVector, so
that destructors aren't needed and we can easily use arena allocation.

I want the same to be true for IceCfgNode as well, but that is a work in
progress.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode106
src/IceInst.h:106: template <> IceOstream &operator<<(IceOstream &Str, const
IceInst &I);
On 2014/04/16 01:27:32, JF wrote:
> I think you want this template specialization in IceDefs.h, otherwise you'll
get
> an ODR violation if IceInst.h ins't also included but the operator is used
with
> IceInst (unlikely since it requires a const IceInst & to do so).

Actually that wasn't supposed to be there anymore.  Gone now.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode109
src/IceInst.h:109: // and the alignment.
On 2014/04/16 01:27:32, JF wrote:
> Are there any special meanings for alignment? What does 0 mean?

This is just the alignment from the LLVM bitcode, which needs to be dealt with
by the target lowering.

> 
> It would be nice to have a higher level list of which src means what for each
of
> the instruction types (instead of having to hunt for each instruction type,
it's
> cleaner in one location). xmacros for this too?

Another approach is to document via simple instruction-specific accessors that
just "return getSrc(0);" or "return getSrc(1);".  (Made these changes where
appropriate.)

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode119
src/IceInst.h:119: static bool classof(const IceInst *Inst) { return
Inst->getKind() == Alloca; }
On 2014/04/16 01:27:32, JF wrote:
> This could also be auto-defined (albeit not declared) through xmacros.

True, except in the next CL, we add "Target" to enum InstKind which is the
starting point for adding target-specific instruction kinds, and the
IceInstTarget::classof() implementation is:
  return Inst->getKind() >= Target;
Can that cleanly fit into x-macros?  (Maybe a flag in the table that selects ==
vs >= operator?)

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode134
src/IceInst.h:134: // Ordered by
http://llvm.org/docs/LangRef.html#binary-operations
On 2014/04/16 01:27:32, JF wrote:
> Why does the order matter? Is the goal to match the order in the NaCl
> BitCodeReader? If so the order isn't guaranteed to be the same.

Order doesn't matter, comment removed.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode158
src/IceInst.h:158: IceInstArithmetic(Cfg, Op, Dest, Source1, Source2);
On 2014/04/16 01:27:32, JF wrote:
> Do the operand types necessarily match?

Right now, I'm only doing the most haphazard of structural checks, and in
particular I'm not checking for legal types.  Should that be done by the bitcode
reader?  Subzero?  The rest of PNaCl?  This needs a higher level decision.

WWPPD?

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode199
src/IceInst.h:199: // unconditional branches.
On 2014/04/16 01:27:32, JF wrote:
> The implementation says that True==False means unconditional. The API should
> explain this.

Done.

> 
> Also, getSrc(0) is the condition?

Yes, IceInstBr::getCondition() added to clarify.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode239
src/IceInst.h:239: IceOperand *CallTarget, bool Tail) {
On 2014/04/16 01:27:32, JF wrote:
> Explain Tail: is it actually tailcall, or LLVM's weird idea of tailcall?

Done.  (It's the weird LLVM tail marker.)

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode270
src/IceInst.h:270: // Ordered by
http://llvm.org/docs/LangRef.html#conversion-operations
On 2014/04/16 01:27:32, JF wrote:
> Ditto on ordering.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode296
src/IceInst.h:296: OpKind CastKind;
On 2014/04/16 01:27:32, JF wrote:
> const, to keep with the other classes.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode304
src/IceInst.h:304: // Ordered by http://llvm.org/docs/LangRef.html#id254
On 2014/04/16 01:27:32, JF wrote:
> Ditto.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode325
src/IceInst.h:325: IceInstFcmp(Cfg, Condition, Dest, Source1, Source2);
On 2014/04/16 01:27:32, JF wrote:
> Do the operand types necessarily match?

Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode336
src/IceInst.h:336: FCond Condition;
On 2014/04/16 01:27:32, JF wrote:
> const, to keep with the other classes.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode344
src/IceInst.h:344: // Ordered by http://llvm.org/docs/LangRef.html#id249
On 2014/04/16 01:27:32, JF wrote:
> Ditto.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode359
src/IceInst.h:359: IceInstIcmp(Cfg, Condition, Dest, Source1, Source2);
On 2014/04/16 01:27:32, JF wrote:
> Do the operand types necessarily match?

Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode370
src/IceInst.h:370: ICond Condition;
On 2014/04/16 01:27:32, JF wrote:
> const, to keep with the other classes.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode373
src/IceInst.h:373: // Load instruction.  The source address is captured in
getSrc(0);
On 2014/04/16 01:27:32, JF wrote:
> Could you infer alignment? We guarantee that FP is naturally aligned, and we
> know the alignment of globals, so you could get better perf out of that
> information.

True, the target lowering can probably make use of that.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode408
src/IceInst.h:408: IceCfgNode **Labels;
On 2014/04/16 01:27:32, JF wrote:
> dtor that deletes Labels.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode413
src/IceInst.h:413: // getSrcSize()==0.
On 2014/04/16 01:27:32, JF wrote:
> Create an isVoid method that does this, so code relying on this pattern is
> easier to read.

Done - hasRetValue().

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode416
src/IceInst.h:416: static IceInstRet *create(IceCfg *Cfg, IceOperand *Source =
NULL) {
On 2014/04/16 01:27:32, JF wrote:
> Source is kind of a bad name here (and in the implementation) since it's the
> returned value.

Done - Source==>RetValue.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode436
src/IceInst.h:436: IceInstSelect(Cfg, Dest, Condition, SourceTrue, SourceFalse);
On 2014/04/16 01:27:32, JF wrote:
> Do the operand's type necessarily match?

Ditto.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode499
src/IceInst.h:499: IceCfgNode *LabelDefault;
On 2014/04/16 01:27:32, JF wrote:
> Why not put default at the start of Labels?
> 
> It would simplify getTerminatorEdges.

It shortens getTerminatorEdges() by 1 line, but needs "+1" sprinkled in several
places.  Plus it wastes a uint64_t if you want Values[i] and Labels[i] to
correspond to each other. :)

I prefer the 2 extra LOC in the current version for its clarity.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode502
src/IceInst.h:502: IceCfgNode **Labels; // size is NumCases
On 2014/04/16 01:27:32, JF wrote:
> dtor that deletes Values and Labels.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.cpp
File src/IceOperand.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.cpp#newc...
src/IceOperand.cpp:55: sprintf(buf, "__%u", getIndex());
On 2014/04/16 01:27:32, JF wrote:
> snprintf

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode12
src/IceOperand.h:12: // LLVM variable that is either register- or
stack-allocated, and the
On 2014/04/16 01:27:32, JF wrote:
> Do variables even have a concept of stack at the IR level? Or are they just
SSA
> values?

I guess you'd say they are SSA values that are ultimately register allocation
candidates.  The distinction between pinned to a register and potentially on the
stack becomes more interesting during the lowering phase, where legal addressing
modes must be maintained.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode27
src/IceOperand.h:27: Constant,
On 2014/04/16 01:27:32, JF wrote:
> What's constant but not Int/FP/Reloc?

Constant is the abstract superclass of all the concrete constant classes.  The
Constant and Constant_Num kinds are used for implementing the LLVM faux RTTI.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode50
src/IceOperand.h:50: virtual void setUse(const IceInst *Inst, const IceCfgNode
*Node) {}
On 2014/04/16 01:27:32, JF wrote:
> Make this pure virtual, error out for constant and keep variable as-is?

Better yet, take this method out of IceOperand and make it non-virtual in
IceVariable only.

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode53
src/IceOperand.h:53: virtual ~IceOperand() {}
On 2014/04/16 01:27:32, JF wrote:
> Ditto on private virtual dtors in derived classes.

Done, though I needed to make ~IceConstant() protected rather than private - LMK
if I'm doing it wrong...

https://codereview.chromium.org/205613002/diff/230001/src/IceOperand.h#newcode56
src/IceOperand.h:56: IceOperand(OperandKind Kind, IceType Type) : Type(Type),
Kind(Kind) {}
On 2014/04/16 01:27:32, JF wrote:
> You should initialize NumVars(0), Vars(NULL) to be safe, or add them as
> arguments to the ctor.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.cpp#newcode30
src/IceTypes.cpp:30: { IceType_f64, 8, "double" }, };
On 2014/04/16 01:27:32, JF wrote:
> I would still use xmacros here, since it prevents this from going out of sync
> with the enum in the .h, and it's pretty regular. That would also make
> iceTypeWidthInBytes simpler.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.cpp#newcode43
src/IceTypes.cpp:43: assert(Width && "Invalid type for iceTypeWidthInBytes()");
On 2014/04/16 01:27:32, JF wrote:
> Is it expected that IceType_void asserts here?

Yes, at least originally.  This function is meant to be called with respect to a
concrete operand which will never have a void type.

But I changed my mind - it's better to make this general and let the caller deal
with a 0 result.  We can still assert here that a valid type has been passed in.

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.h
File src/IceTypes.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceTypes.h#newcode33
src/IceTypes.h:33: template <> IceOstream &operator<<(class IceOstream &Str,
const IceType &Type);
On 2014/04/16 01:27:32, JF wrote:
> Same on this being in IceDefs.h

I think it's OK here.  This operator overload can't be declared without having
enum IceType fully defined.

(Alternatively, I should consider folding IceTypes.h and IceTypes.cpp into
IceDefs.h and some other source file.)

[JF, 2014-04-23 03:51:27]

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode54
src/IceCfg.h:54: IceCfgNode *makeNode(const IceString &Name = "");
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > Your CFG doesn't seem to have an exit node? How do you determine exit, by
> doing
> > RPO and traversing?
> > 
> > I get that any block could have a return instruction, but it seems useful
for
> > some algorithms to have an exit node with edges from each of those node.
> > 
> > Or maybe that's just what I'm used to.
> 
> So far, the only Subzero pass/algorithm that iterates over nodes in non
> arbitrary order is liveness analysis.  However, liveness analysis doesn't need
> to know about exit nodes.  It just has to iterate over the nodes until
> convergence, and convergence may happen sooner iteration happens in reverse
> topological order (and the upstream LLVM tools happen to produce a topological
> sort).
> 
> In any case, the next CL will have a bool IceCfgNode::HasReturn flag in each
> node, to help quickly find nodes that need epilog insertion.  This could be
> generalized to all exit nodes (not just those that need epilogs) and collected
> into IceCfg, but I'd rather hold off until a concrete need arises.

My point is: if each return is succeeded by the exit node then what you describe
isn't needed :-)

I can wait to see what it looks like next time around.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode73
src/IceCfg.h:73: const IceCfgNode *getCurrentNode() const { return CurrentNode;
}
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > What are current nodes in a CFG?
> 
> This is described in the documentation below of the CurrentNode field.  I
added
> a bit of text here as well.
> 
> However, I'm having second thoughts about this whole approach of incrementally
> computing whether variables are entirely block-local, versus "global".  It
might
> be better, less fragile, to construct this information in a separate pass.

As discussed offline, add a TODO to remove the premature optimization later
(since it will be used in the next CL).

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode21
src/IceDefs.h:21: #include <stdio.h> // sprintf
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > Since this is a new code base I'd go with the <c*> headers instead of <*.h>
> > headers for C things:
> >   http://www.gotw.ca/publications/migrating_to_namespaces.htm
> 
> Done for <cassert> and <cstdio>.  stdint.h seems resistant to this change.

What are the issues? I assume something newlib+libc++ related, so I'd like to
fix it. Could you craft a repro and file a bug?

It's actually tested by libc++ and isn't excluded from our test suite:
  test/language.support/cstdint/cstdint.syn/cstdint.pass.cpp
So it's probably not a known issue.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:89: std::vector<IceConstantDouble *> DoublePool;
> > It's also weird to use float and double as keys in a map, you're going to
have
> a
> > bad time e.g. with NaN. You want to make sure you preserve all bits of
> > information provided by the program, but I assume you also want to do some
> > partial evaluation / constant propagation at some point?
> 
> In the updated code, memcmp is used for the comparison.  There is a TODO to
> allow a custom comparator for when the tuple contains fields like non-pooled
> strings.

Could you make sure the tests have NaN values in the constant pool?

> BTW, think the consequence of using FP keys with ordinary comparisons would be
> that no matches would be found and you'd end up with duplicates in the pool.

It would be undefined: AFAIK std::map needs to have ordered keys, and NaNs
aren't ordered. Insertion would happen at the wrong place and the map would be
imbalanced. It could potentially cause all subsequent insertions, of any value,
to force duplication.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:89: std::vector<IceConstantDouble *> DoublePool;
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > I find the map/vector duality really weird: the map is from the value you
want
> > to have in the constant pool to the index number at which it was added, and
> the
> > vector then has one entry per index.
> > 
> > Why not just use the map?
> 
> The latest patchset makes this map/vector duality less obvious, but the issue
is
> still there.  What's happening is that the map holds the tuple that is used to
> construct the the IceConstant object, and the vector holds the actual
> IceConstant object.

The whole duality still seems weird. I really think having a single container
would be better. Why not just have an std::deque and search it linearly? There
aren't really that many constant pool entries. The code would be much simpler
(though it still has to handle the NaN issue), and if it does become a
bottleneck then I suspect map+vector won't be the right solution either.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode61
src/IceInst.h:61: assert(0);
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > ?
> 
> Jan had the same comment. :)  Added a comment to explain that legitimate
> terminator instructions override this method with a non-asserting
> implementation.

llvm_unreachable would be better for this.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode109
src/IceInst.h:109: // and the alignment.
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > Are there any special meanings for alignment? What does 0 mean?
> 
> This is just the alignment from the LLVM bitcode, which needs to be dealt with
> by the target lowering.

Could you add a comment to this code that explains alignment? I'd rather not
point back to LLVM.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode119
src/IceInst.h:119: static bool classof(const IceInst *Inst) { return
Inst->getKind() == Alloca; }
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > This could also be auto-defined (albeit not declared) through xmacros.
> 
> True, except in the next CL, we add "Target" to enum InstKind which is the
> starting point for adding target-specific instruction kinds, and the
> IceInstTarget::classof() implementation is:
>   return Inst->getKind() >= Target;
> Can that cleanly fit into x-macros?  (Maybe a flag in the table that selects
==
> vs >= operator?)

I see. Maybe, but probably not worth it.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode158
src/IceInst.h:158: IceInstArithmetic(Cfg, Op, Dest, Source1, Source2);
On 2014/04/21 20:26:40, stichnot wrote:
> On 2014/04/16 01:27:32, JF wrote:
> > Do the operand types necessarily match?
> 
> Right now, I'm only doing the most haphazard of structural checks, and in
> particular I'm not checking for legal types.  Should that be done by the
bitcode
> reader?  Subzero?  The rest of PNaCl?  This needs a higher level decision.
> 
> WWPPD?

Good point, probably the reader, but I'm not sure that's an explicit requirement
so you should probably sync with Karl on this.

https://codereview.chromium.org/205613002/diff/270001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceCfg.cpp#newcode44
src/IceCfg.cpp:44: // name.  The Node argument is the node where the variable is
defined.
Silly comment... Two spaces after a period? You know the code review is winding
down at this point ;-)

https://codereview.chromium.org/205613002/diff/270001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:28: if (Name != "")
!Name.empty()

https://codereview.chromium.org/205613002/diff/270001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:30: const static size_t BufLen = 30;
Here and other places: LLVM has array_lengthof with you could just use in
snprintf (and do away with BufLen).

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode40
src/IceDefs.h:40: class IceCfg;
Why not shed the Ice (or melt?) for Cfg and the other types in this file? Seems
cleaner, since it's already in the namespace there can't really be a name
collision (except if you start using namespace, which is a bad idea anyways).

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode58
src/IceDefs.h:58: typedef uint32_t IceSize_t;
Names ending in _t are reserved by POSIX. Why not just Size?

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode62
src/IceDefs.h:62: template <typename T, typename Cmp = std::less<T> > class
ValueTranslation {
You need to #include <functional> to use std::less.

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode64
src/IceDefs.h:64: typedef typename std::map<const T, IceSize_t, Cmp>
ContainerType;
Just to restore my template sanity, could you use <type_traits> and throw a
static_assert here:
  static_assert(!std::is_floating_point<T>::value, "can't index ValueTranslation
with a floating-point value");

If it were necessary Cmp could be made to handle floating-point values properly,
but I don't think it's useful in this case.

https://codereview.chromium.org/205613002/diff/270001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:50: if (A.first != B.first)
Ditto on static_assert for !FP.

https://codereview.chromium.org/205613002/diff/270001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:132: }
Could you add tests for this function? It gave me a mild headache, but I think
it's correct :)

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode38
src/IceInst.cpp:38: }
Same thing on deletion, though it kind of seems like you want an owning pointer
or something of the kind (no refcount, just a dtor delete) in the contained
values.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode75
src/IceInst.cpp:75: switch (getOp()) { ICEINSTARITHMETIC_TABLE }
Load from a static const bool table instead: show those branches some hate (and
assume the compiler won't emit a table for your switch). It also usually makes
your final binary slightly smaller.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode78
src/IceInst.cpp:78: assert(0); // should be unreachable
llvm_unreachable

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode267
src/IceInst.cpp:267: switch (getOp()) { ICEINSTARITHMETIC_TABLE }
Same thing on table lookup from static const char *.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode329
src/IceInst.cpp:329: ICEINSTCAST_TABLE
Same.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode352
src/IceInst.cpp:352: switch (getCondition()) { ICEINSTICMP_TABLE }
Same.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode368
src/IceInst.cpp:368: switch (getCondition()) { ICEINSTFCMP_TABLE }
Same.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode141
src/IceInst.h:141: X(And, "and", true) X(Or, "or", true) X(Xor, "xor", true)
Yeah, un-clang-formatting this would be nice.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode246
src/IceInst.h:246: // call must be executed as a tail call).
What does it mean?

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode280
src/IceInst.h:280: X(Uitofp, "uitofp") X(Sitofp, "sitofp") X(Bitcast, "bitcast")
clang-un-format

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode313
src/IceInst.h:313: X(True, "true")
clang-un-format

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode346
src/IceInst.h:346: X(Ule, "ule") X(Sgt, "sgt") X(Sge, "sge") X(Slt, "slt")
X(Sle, "sle")
clang-un-format

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode410
src/IceInst.h:410: delete[] Labels;
Call you call Cfg->deleteInst. That allows you to remove the preprocessor use,
and prevents asymmetries in new/delete versus new[]/delete[] (or malloc/free)
since both functions are implemented next to each other.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode518
src/IceInst.h:518: delete[] Labels;
Same on delete.

https://codereview.chromium.org/205613002/diff/270001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceTypes.cpp#newcode27
src/IceTypes.cpp:27: IceString DisplayString;
This should be a POD since it's global, const char * is probably the best
choice.

[Jim Stichnoth, 2014-04-26 15:02:10]

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode54
src/IceCfg.h:54: IceCfgNode *makeNode(const IceString &Name = "");
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > Your CFG doesn't seem to have an exit node? How do you determine exit, by
> > doing
> > > RPO and traversing?
> > > 
> > > I get that any block could have a return instruction, but it seems useful
> for
> > > some algorithms to have an exit node with edges from each of those node.
> > > 
> > > Or maybe that's just what I'm used to.
> > 
> > So far, the only Subzero pass/algorithm that iterates over nodes in non
> > arbitrary order is liveness analysis.  However, liveness analysis doesn't
need
> > to know about exit nodes.  It just has to iterate over the nodes until
> > convergence, and convergence may happen sooner iteration happens in reverse
> > topological order (and the upstream LLVM tools happen to produce a
topological
> > sort).
> > 
> > In any case, the next CL will have a bool IceCfgNode::HasReturn flag in each
> > node, to help quickly find nodes that need epilog insertion.  This could be
> > generalized to all exit nodes (not just those that need epilogs) and
collected
> > into IceCfg, but I'd rather hold off until a concrete need arises.
> 
> My point is: if each return is succeeded by the exit node then what you
describe
> isn't needed :-)
> 
> I can wait to see what it looks like next time around.

OK, I think what you're saying/suggesting would be absolutely true if lowered
ICE were guaranteed to have a single epilog.  For now, I'm allowing multiple
epilogs in a function until we find a reason to restrict them.

https://codereview.chromium.org/205613002/diff/230001/src/IceCfg.h#newcode73
src/IceCfg.h:73: const IceCfgNode *getCurrentNode() const { return CurrentNode;
}
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > What are current nodes in a CFG?
> > 
> > This is described in the documentation below of the CurrentNode field.  I
> added
> > a bit of text here as well.
> > 
> > However, I'm having second thoughts about this whole approach of
incrementally
> > computing whether variables are entirely block-local, versus "global".  It
> might
> > be better, less fragile, to construct this information in a separate pass.
> 
> As discussed offline, add a TODO to remove the premature optimization later
> (since it will be used in the next CL).

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode21
src/IceDefs.h:21: #include <stdio.h> // sprintf
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > Since this is a new code base I'd go with the <c*> headers instead of
<*.h>
> > > headers for C things:
> > >   http://www.gotw.ca/publications/migrating_to_namespaces.htm
> > 
> > Done for <cassert> and <cstdio>.  stdint.h seems resistant to this change.
> 
> What are the issues? I assume something newlib+libc++ related, so I'd like to
> fix it. Could you craft a repro and file a bug?
> 
> It's actually tested by libc++ and isn't excluded from our test suite:
>   test/language.support/cstdint/cstdint.syn/cstdint.pass.cpp
> So it's probably not a known issue.

It looks like <cstdint> is C++11, at least according to gcc 4.6.3.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:89: std::vector<IceConstantDouble *> DoublePool;
On 2014/04/23 03:51:28, JF wrote:
> > > It's also weird to use float and double as keys in a map, you're going to
> have
> > a
> > > bad time e.g. with NaN. You want to make sure you preserve all bits of
> > > information provided by the program, but I assume you also want to do some
> > > partial evaluation / constant propagation at some point?
> > 
> > In the updated code, memcmp is used for the comparison.  There is a TODO to
> > allow a custom comparator for when the tuple contains fields like non-pooled
> > strings.
> 
> Could you make sure the tests have NaN values in the constant pool?

Done - see fpconst.pnacl.ll, which includes duplicate float and double constants
both within a method and across methods.

https://codereview.chromium.org/205613002/diff/230001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:89: std::vector<IceConstantDouble *> DoublePool;
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > I find the map/vector duality really weird: the map is from the value you
> want
> > > to have in the constant pool to the index number at which it was added,
and
> > the
> > > vector then has one entry per index.
> > > 
> > > Why not just use the map?
> > 
> > The latest patchset makes this map/vector duality less obvious, but the
issue
> is
> > still there.  What's happening is that the map holds the tuple that is used
to
> > construct the the IceConstant object, and the vector holds the actual
> > IceConstant object.
> 
> The whole duality still seems weird. I really think having a single container
> would be better. Why not just have an std::deque and search it linearly? There
> aren't really that many constant pool entries. The code would be much simpler
> (though it still has to handle the NaN issue), and if it does become a
> bottleneck then I suspect map+vector won't be the right solution either.

You are right - it's good enough to keep a simple container mapping a tuple of
ingredients to fully baked and pooled objects.  This is now done, including a
flag to select between the default < operator and a memcmp-based type punning
comparison.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode61
src/IceInst.h:61: assert(0);
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > ?
> > 
> > Jan had the same comment. :)  Added a comment to explain that legitimate
> > terminator instructions override this method with a non-asserting
> > implementation.
> 
> llvm_unreachable would be better for this.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode109
src/IceInst.h:109: // and the alignment.
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > Are there any special meanings for alignment? What does 0 mean?
> > 
> > This is just the alignment from the LLVM bitcode, which needs to be dealt
with
> > by the target lowering.
> 
> Could you add a comment to this code that explains alignment? I'd rather not
> point back to LLVM.

Done.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode119
src/IceInst.h:119: static bool classof(const IceInst *Inst) { return
Inst->getKind() == Alloca; }
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > This could also be auto-defined (albeit not declared) through xmacros.
> > 
> > True, except in the next CL, we add "Target" to enum InstKind which is the
> > starting point for adding target-specific instruction kinds, and the
> > IceInstTarget::classof() implementation is:
> >   return Inst->getKind() >= Target;
> > Can that cleanly fit into x-macros?  (Maybe a flag in the table that selects
> ==
> > vs >= operator?)
> 
> I see. Maybe, but probably not worth it.

OK, deferring this for now.

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.h#newcode158
src/IceInst.h:158: IceInstArithmetic(Cfg, Op, Dest, Source1, Source2);
On 2014/04/23 03:51:28, JF wrote:
> On 2014/04/21 20:26:40, stichnot wrote:
> > On 2014/04/16 01:27:32, JF wrote:
> > > Do the operand types necessarily match?
> > 
> > Right now, I'm only doing the most haphazard of structural checks, and in
> > particular I'm not checking for legal types.  Should that be done by the
> bitcode
> > reader?  Subzero?  The rest of PNaCl?  This needs a higher level decision.
> > 
> > WWPPD?
> 
> Good point, probably the reader, but I'm not sure that's an explicit
requirement
> so you should probably sync with Karl on this.

Done.  TODO added to the top of this file.

https://codereview.chromium.org/205613002/diff/270001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceCfg.cpp#newcode44
src/IceCfg.cpp:44: // name.  The Node argument is the node where the variable is
defined.
On 2014/04/23 03:51:28, JF wrote:
> Silly comment... Two spaces after a period? You know the code review is
winding
> down at this point ;-)

Two spaces is how I always write prose... maybe file a clang-format bug? :)

https://codereview.chromium.org/205613002/diff/270001/src/IceCfgNode.cpp
File src/IceCfgNode.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:28: if (Name != "")
On 2014/04/23 03:51:28, JF wrote:
> !Name.empty()

Done (here and 2 other places).

https://codereview.chromium.org/205613002/diff/270001/src/IceCfgNode.cpp#newc...
src/IceCfgNode.cpp:30: const static size_t BufLen = 30;
On 2014/04/23 03:51:28, JF wrote:
> Here and other places: LLVM has array_lengthof with you could just use in
> snprintf (and do away with BufLen).

Done.  I couldn't make this work in IceGlobalContext.cpp where the array size
depends on input arguments, let me know if you think that should be possible.

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode40
src/IceDefs.h:40: class IceCfg;
On 2014/04/23 03:51:28, JF wrote:
> Why not shed the Ice (or melt?) for Cfg and the other types in this file?
Seems
> cleaner, since it's already in the namespace there can't really be a name
> collision (except if you start using namespace, which is a bad idea anyways).

Done.  Except I'd prefer to keep IceString, since String is such a generic name.

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode58
src/IceDefs.h:58: typedef uint32_t IceSize_t;
On 2014/04/23 03:51:28, JF wrote:
> Names ending in _t are reserved by POSIX. Why not just Size?

Can we compromise on SizeT? :)

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode62
src/IceDefs.h:62: template <typename T, typename Cmp = std::less<T> > class
ValueTranslation {
On 2014/04/23 03:51:28, JF wrote:
> You need to #include <functional> to use std::less.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceDefs.h#newcode64
src/IceDefs.h:64: typedef typename std::map<const T, IceSize_t, Cmp>
ContainerType;
On 2014/04/23 03:51:28, JF wrote:
> Just to restore my template sanity, could you use <type_traits> and throw a
> static_assert here:
>   static_assert(!std::is_floating_point<T>::value, "can't index
ValueTranslation
> with a floating-point value");
> 
> If it were necessary Cmp could be made to handle floating-point values
properly,
> but I don't think it's useful in this case.

Done (sort of).  Sadly, static_assert and is_floating_point aren't available
until C++11, so the useless LLVM_STATIC_ASSERT is left as a placeholder.

https://codereview.chromium.org/205613002/diff/270001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:50: if (A.first != B.first)
On 2014/04/23 03:51:28, JF wrote:
> Ditto on static_assert for !FP.

This isn't necessary, since the type of A.first and B.first is always IceType
which is an enum.

https://codereview.chromium.org/205613002/diff/270001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:132: }
On 2014/04/23 03:51:28, JF wrote:
> Could you add tests for this function? It gave me a mild headache, but I think
> it's correct :)

How about a TODO for the next CL?  I probably should have held this method back
for this CL since there aren't any actual callers yet.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode38
src/IceInst.cpp:38: }
On 2014/04/23 03:51:28, JF wrote:
> Same thing on deletion, though it kind of seems like you want an owning
pointer
> or something of the kind (no refcount, just a dtor delete) in the contained
> values.

Done.  And you're right, it was just wrong for the destructor to be deleting
objects that it didn't create.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode75
src/IceInst.cpp:75: switch (getOp()) { ICEINSTARITHMETIC_TABLE }
On 2014/04/23 03:51:28, JF wrote:
> Load from a static const bool table instead: show those branches some hate
(and
> assume the compiler won't emit a table for your switch). It also usually makes
> your final binary slightly smaller.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode78
src/IceInst.cpp:78: assert(0); // should be unreachable
On 2014/04/23 03:51:28, JF wrote:
> llvm_unreachable

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode267
src/IceInst.cpp:267: switch (getOp()) { ICEINSTARITHMETIC_TABLE }
On 2014/04/23 03:51:28, JF wrote:
> Same thing on table lookup from static const char *.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode329
src/IceInst.cpp:329: ICEINSTCAST_TABLE
On 2014/04/23 03:51:28, JF wrote:
> Same.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode352
src/IceInst.cpp:352: switch (getCondition()) { ICEINSTICMP_TABLE }
On 2014/04/23 03:51:28, JF wrote:
> Same.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.cpp#newcode368
src/IceInst.cpp:368: switch (getCondition()) { ICEINSTFCMP_TABLE }
On 2014/04/23 03:51:28, JF wrote:
> Same.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode141
src/IceInst.h:141: X(And, "and", true) X(Or, "or", true) X(Xor, "xor", true)
On 2014/04/23 03:51:28, JF wrote:
> Yeah, un-clang-formatting this would be nice.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode246
src/IceInst.h:246: // call must be executed as a tail call).
On 2014/04/23 03:51:28, JF wrote:
> What does it mean?

Per offline discussion - since the tail marker isn't being used in Subzero
currently, and its meaning within PNaCl isn't completely clear, I deleted it
here.  It will be easy to add back if we need it.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode280
src/IceInst.h:280: X(Uitofp, "uitofp") X(Sitofp, "sitofp") X(Bitcast, "bitcast")
On 2014/04/23 03:51:28, JF wrote:
> clang-un-format

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode313
src/IceInst.h:313: X(True, "true")
On 2014/04/23 03:51:28, JF wrote:
> clang-un-format

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode346
src/IceInst.h:346: X(Ule, "ule") X(Sgt, "sgt") X(Sge, "sge") X(Slt, "slt")
X(Sle, "sle")
On 2014/04/23 03:51:28, JF wrote:
> clang-un-format

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode410
src/IceInst.h:410: delete[] Labels;
On 2014/04/23 03:51:28, JF wrote:
> Call you call Cfg->deleteInst. That allows you to remove the preprocessor use,
> and prevents asymmetries in new/delete versus new[]/delete[] (or malloc/free)
> since both functions are implemented next to each other.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceInst.h#newcode518
src/IceInst.h:518: delete[] Labels;
On 2014/04/23 03:51:28, JF wrote:
> Same on delete.

Done.

https://codereview.chromium.org/205613002/diff/270001/src/IceTypes.cpp
File src/IceTypes.cpp (right):

https://codereview.chromium.org/205613002/diff/270001/src/IceTypes.cpp#newcode27
src/IceTypes.cpp:27: IceString DisplayString;
On 2014/04/23 03:51:28, JF wrote:
> This should be a POD since it's global, const char * is probably the best
> choice.

Done.

[JF, 2014-04-26 20:20:55]

lgtm after a few cosmetic additions below.

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceDefs.h#newcode21
src/IceDefs.h:21: #include <stdio.h> // sprintf
On 2014/04/26 15:02:11, stichnot wrote:
> On 2014/04/23 03:51:28, JF wrote:
> > On 2014/04/21 20:26:40, stichnot wrote:
> > > On 2014/04/16 01:27:32, JF wrote:
> > > > Since this is a new code base I'd go with the <c*> headers instead of
> <*.h>
> > > > headers for C things:
> > > >   http://www.gotw.ca/publications/migrating_to_namespaces.htm
> > > 
> > > Done for <cassert> and <cstdio>.  stdint.h seems resistant to this change.
> > 
> > What are the issues? I assume something newlib+libc++ related, so I'd like
to
> > fix it. Could you craft a repro and file a bug?
> > 
> > It's actually tested by libc++ and isn't excluded from our test suite:
> >   test/language.support/cstdint/cstdint.syn/cstdint.pass.cpp
> > So it's probably not a known issue.
> 
> It looks like <cstdint> is C++11, at least according to gcc 4.6.3.

Hah, I did not know that.

https://codereview.chromium.org/205613002/diff/290001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceCfg.h#newcode109
src/IceCfg.h:109: }
Same as before: this should call Allocator.deallocate(ptr, size), and *those*
shouldn't do anything for the bump allocator (the implementation is indeed just
an empty function).

I want you to be able to s/BumpPtrAllocator/JimsAwesomeOAllocator/ below and not
have to do anything to get awesome-o-allocator working correctly.

https://codereview.chromium.org/205613002/diff/290001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceDefs.h#newcode19
src/IceDefs.h:19: #include <stdint.h>
TODO C++11 ctsdint

https://codereview.chromium.org/205613002/diff/290001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:51: if (A.first != B.first)
.first could also potentially be FP, so you should probably handle it the same
as .second.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode35
src/IceInst.cpp:35: sizeof(InstArithmeticAttributes) /
sizeof(*InstArithmeticAttributes);
array_lengthof

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode47
src/IceInst.cpp:47: sizeof(InstCastAttributes) / sizeof(*InstCastAttributes);
ditto

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode59
src/IceInst.cpp:59: sizeof(InstFcmpAttributes) / sizeof(*InstFcmpAttributes);
ditto

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode71
src/IceInst.cpp:71: sizeof(InstIcmpAttributes) / sizeof(*InstIcmpAttributes);
ditto

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode99
src/IceInst.cpp:99: assert(Align == 0 || !(Align & (Align - 1)));
llvm::isPowerOf2_32

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode376
src/IceInst.cpp:376: break;
Won't you need to factor alignment out at some point (DefaultAlign(Type) or
something)?

Maybe for the later CL, but dump could just call that function.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.h#newcode134
src/IceInst.h:134: uint32_t getAlign() const { return Align; }
Also rename this and the member variable below to be AlignInBytes.

https://codereview.chromium.org/205613002/diff/290001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fpconst.pnacl.ll (right):

https://codereview.chromium.org/205613002/diff/290001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fpconst.pnacl.ll:1: ; RUN: %szdiff --llvm2ice=%llvm2ice
%s | FileCheck --check-prefix=DUMP %s
Add a comment on what the hex value FP constants are.

[Jim Stichnoth, 2014-04-27 15:04:57]

All comments should be addressed, except for the issue of being unable to use
llvm::array_lengthof() instead of sizeof(MyArray)/sizeof(*MyArray) .

https://codereview.chromium.org/205613002/diff/290001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceCfg.h#newcode109
src/IceCfg.h:109: }
On 2014/04/26 20:20:56, JF wrote:
> Same as before: this should call Allocator.deallocate(ptr, size), and *those*
> shouldn't do anything for the bump allocator (the implementation is indeed
just
> an empty function).
> 
> I want you to be able to s/BumpPtrAllocator/JimsAwesomeOAllocator/ below and
not
> have to do anything to get awesome-o-allocator working correctly.

Ah, of course.  Done.

https://codereview.chromium.org/205613002/diff/290001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceDefs.h#newcode19
src/IceDefs.h:19: #include <stdint.h>
On 2014/04/26 20:20:56, JF wrote:
> TODO C++11 ctsdint

Done.

https://codereview.chromium.org/205613002/diff/290001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:51: if (A.first != B.first)
On 2014/04/26 20:20:56, JF wrote:
> .first could also potentially be FP, so you should probably handle it the same
> as .second.

No - A.first and B.first are of type "enum Type" (formerly known as enum
IceType).  A.second and B.second have the templated type KeyType which might be
FP, but .first is not a templated type and is always an enum type which can be
compared naturally.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode35
src/IceInst.cpp:35: sizeof(InstArithmeticAttributes) /
sizeof(*InstArithmeticAttributes);
On 2014/04/26 20:20:56, JF wrote:
> array_lengthof

Strangely, this doesn't work.

const size_t InstArithmeticAttributesSize = 
llvm::array_lengthof(InstArithmeticAttributes);

src/IceInst.cpp:35:48: error: no matching function for call to
‘array_lengthof(const Ice::{anonymous}::<anonymous struct> [18])’
src/IceInst.cpp:35:48: note: candidate is:
/ssd/stichnot/nacltest/native_client/toolchain_build/src/llvm/include/llvm/ADT/STLExtras.h:250:15:
note: template<class T, unsigned int N> size_t llvm::array_lengthof(T (&)[N])

I see the "[18]" right there, so, wat?

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode99
src/IceInst.cpp:99: assert(Align == 0 || !(Align & (Align - 1)));
On 2014/04/26 20:20:56, JF wrote:
> llvm::isPowerOf2_32

Done.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode376
src/IceInst.cpp:376: break;
On 2014/04/26 20:20:56, JF wrote:
> Won't you need to factor alignment out at some point (DefaultAlign(Type) or
> something)?
> 
> Maybe for the later CL, but dump could just call that function.

Good point.  I added alignment to the IceTypes.def x-macros, along with
typeAlignInBytes() similar to typeWidthInBytes(), and used this for the dump
routines.

For x86 lowering, it's safe to ignore alignment, except perhaps for SIMD.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.h#newcode134
src/IceInst.h:134: uint32_t getAlign() const { return Align; }
On 2014/04/26 20:20:56, JF wrote:
> Also rename this and the member variable below to be AlignInBytes.

Done.

https://codereview.chromium.org/205613002/diff/290001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/fpconst.pnacl.ll (right):

https://codereview.chromium.org/205613002/diff/290001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/fpconst.pnacl.ll:1: ; RUN: %szdiff --llvm2ice=%llvm2ice
%s | FileCheck --check-prefix=DUMP %s
On 2014/04/26 20:20:56, JF wrote:
> Add a comment on what the hex value FP constants are.

Done.

[Jim Stichnoth, 2014-04-27 15:25:22]

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode35
src/IceInst.cpp:35: sizeof(InstArithmeticAttributes) /
sizeof(*InstArithmeticAttributes);
On 2014/04/27 15:04:57, stichnot wrote:
> On 2014/04/26 20:20:56, JF wrote:
> > array_lengthof
> 
> Strangely, this doesn't work.
> 
> const size_t InstArithmeticAttributesSize = 
> llvm::array_lengthof(InstArithmeticAttributes);
> 
> src/IceInst.cpp:35:48: error: no matching function for call to
> ‘array_lengthof(const Ice::{anonymous}::<anonymous struct> [18])’
> src/IceInst.cpp:35:48: note: candidate is:
>
/ssd/stichnot/nacltest/native_client/toolchain_build/src/llvm/include/llvm/ADT/STLExtras.h:250:15:
> note: template<class T, unsigned int N> size_t llvm::array_lengthof(T (&)[N])
> 
> I see the "[18]" right there, so, wat?

Done.  Turns out you can't use array_lengthof for anonymous structs.  Wat?

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode47
src/IceInst.cpp:47: sizeof(InstCastAttributes) / sizeof(*InstCastAttributes);
On 2014/04/26 20:20:56, JF wrote:
> ditto

Done.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode59
src/IceInst.cpp:59: sizeof(InstFcmpAttributes) / sizeof(*InstFcmpAttributes);
On 2014/04/26 20:20:56, JF wrote:
> ditto

Done.

https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode71
src/IceInst.cpp:71: sizeof(InstIcmpAttributes) / sizeof(*InstIcmpAttributes);
On 2014/04/26 20:20:56, JF wrote:
> ditto

Done.

[JF, 2014-04-27 15:59:44]

>
https://codereview.chromium.org/205613002/diff/290001/src/IceInst.cpp#newcode35
> src/IceInst.cpp:35: sizeof(InstArithmeticAttributes) /
> sizeof(*InstArithmeticAttributes);
> On 2014/04/27 15:04:57, stichnot wrote:
>>
>> On 2014/04/26 20:20:56, JF wrote:
>> > array_lengthof
>
>
>> Strangely, this doesn't work.
>
>
>> const size_t InstArithmeticAttributesSize =
>> llvm::array_lengthof(InstArithmeticAttributes);
>
>
>> src/IceInst.cpp:35:48: error: no matching function for call to
>> 'array_lengthof(const Ice::{anonymous}::<anonymous struct> [18])'
>> src/IceInst.cpp:35:48: note: candidate is:
>
>
>
/ssd/stichnot/nacltest/native_client/toolchain_build/src/llvm/include/llvm/ADT/STLExtras.h:250:15:
>>
>> note: template<class T, unsigned int N> size_t llvm::array_lengthof(T
>
> (&)[N])
>
>> I see the "[18]" right there, so, wat?
>
>
> Done.  Turns out you can't use array_lengthof for anonymous structs.
> Wat?

Oh right... Templates don't like them as arguments. Same for function local
types.

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2014-04-27 23:39:44]

The CQ bit was checked by stichnot@chromium.org

[commit-bot: I haz the power, 2014-04-27 23:40:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-27 23:40:06]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[Jim Stichnoth, 2014-04-29 17:52:46]

Committed patchset #16 manually as rf7c9a14 (presubmit successful).

[Jim Stichnoth, 2015-09-25 17:23:10]

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp
File src/IceInst.cpp (right):

https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode54
src/IceInst.cpp:54: case Fmul:
On 2014/04/16 01:27:32, JF wrote:
> Fadd and Fmul aren't commutative.

LLVM disagrees... see Instruction::isCommutative() in
llvm/lib/IR/Instruction.cpp

[JF, 2015-09-25 17:48:51]

On 2015/09/25 17:23:10, stichnot wrote:
> https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp
> File src/IceInst.cpp (right):
> 
>
https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode54
> src/IceInst.cpp:54: case Fmul:
> On 2014/04/16 01:27:32, JF wrote:
> > Fadd and Fmul aren't commutative.
> 
> LLVM disagrees... see Instruction::isCommutative() in
> llvm/lib/IR/Instruction.cpp

The following thread explains a bit:
  http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20121126/157063.html

Eventually the code got refactored to work in concert with:
 
https://github.com/llvm-mirror/llvm/blob/037cfdbe31069696574cfaac906f7bc59b5a...

It's always reassociative *unless* there are NaNs. Fast-math says there can't be
NaNs, so under fast-math it is reassociative. Without fast-math it isn't!

[JF, 2015-09-25 17:52:15]

On 2015/09/25 17:48:51, JF wrote:
> On 2015/09/25 17:23:10, stichnot wrote:
> > https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp
> > File src/IceInst.cpp (right):
> > 
> >
>
https://codereview.chromium.org/205613002/diff/230001/src/IceInst.cpp#newcode54
> > src/IceInst.cpp:54: case Fmul:
> > On 2014/04/16 01:27:32, JF wrote:
> > > Fadd and Fmul aren't commutative.
> > 
> > LLVM disagrees... see Instruction::isCommutative() in
> > llvm/lib/IR/Instruction.cpp
> 
> The following thread explains a bit:
>  
http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20121126/157063.html
> 
> Eventually the code got refactored to work in concert with:
>  
>
https://github.com/llvm-mirror/llvm/blob/037cfdbe31069696574cfaac906f7bc59b5a...
> 
> It's always reassociative *unless* there are NaNs. Fast-math says there can't
be
> NaNs, so under fast-math it is reassociative. Without fast-math it isn't!

s/reassociative/commutative/

[JF, 2015-09-25 18:07:45]

OK ignoring my morning confusion, it turns out you're right:

  http://grouper.ieee.org/groups/1788/email/msg03558.html
The clause of 754 at work is 6.2.3 NaN propagation:
"If two or more inputs are NaN, then the payload of the resulting NaN should be
identical to the payload of one of the input NaNs if representable in the
destination format. This standard does not specify which of the input NaNs will
provide the payload."


and:

  http://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html
"If both operands are NaNs, then the result will be one of those NaNs, but it
might not be the NaN that was generated first."


Sorry for the noise!