Add Om1 lowering with no optimizations

Add Om1 lowering with no optimizations.

This adds infrastructure for low-level x86-32 instructions, and the target lowering patterns.

Practically no optimizations are performed.  Optimizations to be introduced later include liveness analysis, dead-code elimination, global linear-scan register allocation, linear-scan based stack slot coalescing, and compare/branch fusing.  One optimization that is present is simple coalescing of stack slots for variables that are only live within a single basic block.

There are also some fairly comprehensive cross tests.  This testing infrastructure translates bitcode using both Subzero and llc, and a testing harness calls both versions with a variety of "interesting" inputs and compares the results.  Specifically, Arithmetic, Icmp, Fcmp, and Cast instructions are tested this way, across all PNaCl primitive types.

BUG=
R=jvoung@chromium.org

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

[Jim Stichnoth, 2014-04-30 22:28:18]

This is the second mega-CL for Subzero.  The rest should be much more
manageable, at least until we start adding more targets beyond x86-32.

I still need to update the lit "smoke tests" to reflect the code emitted by the
Om1 optimization level.  Also, I'd like to refactor some of the cross tests
using templates to make the code much less repetitive.

JF: Please add some of your C++ magic.

Derek, Jan: Please look at the x86-32 low-level instructions, and the x86-32
target lowering patterns.

Mark: Please look at the tests for completeness and overall goodness.

[JF, 2014-05-01 00:16:54]

Partial first pass.

https://codereview.chromium.org/265703002/diff/1/Makefile
File Makefile (right):

https://codereview.chromium.org/265703002/diff/1/Makefile#newcode32
Makefile:32: CXXFLAGS := -Wall -Wextra -Werror -fno-rtti -fno-exceptions \
That's bold. Kudos if you can keep -Wextra, but I've seen it complain about
really annoying stuff in the past!

https://codereview.chromium.org/265703002/diff/1/Makefile#newcode70
Makefile:70: (cd crosstest; LLVM_BIN_PATH=$(LLVM_BIN_PATH) ./runtests.sh)
When will it be appropriate to add test bots to subzero?

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:17: argparser.add_argument('--target', required=False,
default='X8632',
X8632 isn't the naming you followed in the previous CL?

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:20: choices=['m1', '-1', '0', '1', '2'],
Are m1 and -1 the same?

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:39: shellcmd('sed -i
"s/le32-unknown-nacl/i686-pc-linux-gnu/" ' + bitcode)
sed?

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:50: shellcmd(['$LLVM_BIN_PATH/llvm-mc',
Shouldn't LLVM_BIN_PATH be an argument instead?

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:51: '-arch=x86',
x86 always?

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:62: #objs.append(obj_llc)
Commented out.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:66: ['-lm', '-o', os.path.join(args.dir, args.output)])
Doesn't this sometimes need to be clang++ to find the C++ libraries?

https://codereview.chromium.org/265703002/diff/1/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/runtests.sh#newcode2
crosstest/runtests.sh:2: 
Comment explaining when to add tests here. Maybe testing should be automated
through glob?

https://codereview.chromium.org/265703002/diff/1/crosstest/runtests.sh#newcode3
crosstest/runtests.sh:3: set -x
You should also set -e and set -u, and then you can drop the || exit 1 below.

https://codereview.chromium.org/265703002/diff/1/crosstest/simple_loop.c
File crosstest/simple_loop.c (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/simple_loop.c#newc...
crosstest/simple_loop.c:1: int simple_loop(int *a, int n) {
Comment explaining test.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp
File crosstest/test_arith.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp#new...
crosstest/test_arith.cpp:4: 
+1 on templates and macros :D

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp#new...
crosstest/test_arith.cpp:9: bool testAdd(bool a, bool b) { return a + b; }
I'm not sure it's worth testing none-bitwise operations on bool.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cpp
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:3: 
I think that this test could be partly evaluated at compile-time when built with
LTO. You probably want to make the access to Values be volatile, or create a
no-inline getter to Values, so that the compiler can't cleverly optimize the
test away.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:10: }
Ew. That is quite ugly... and kind of clever. Could you explain it?

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:19: const static unsigned NumValues =
sizeof(Values) / sizeof(*Values);
size_t, same on loop bounds.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:27: bool DisallowIllegal; // for divide related
tests
Could you instead figure out that the test is illegal from its name? You already
check the value. You're also exercising undefined behavior with the shift by
larger than / equal to bitwidth.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:49: };
xmacros!!! It seems like the test declaration/definition/registration could all
be folded together, and it would be more readable and unlikely to diverge that
way.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:216: }
It seems like the 3 loops above are pretty much the same thing. Could you factor
it out?

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.cpp
File crosstest/test_cast.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.cpp#newc...
crosstest/test_cast.cpp:8: };
Couldn't this avoid the class, and just be a templated function?

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.cpp#newc...
crosstest/test_cast.cpp:35: // icmp back to trunc or fptoui.
TODO

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.h
File crosstest/test_cast.h (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.h#newcode13
crosstest/test_cast.h:13: signed char castUi64ToSi8(uint64_t a);
Why not use the [u]int_t typedefs here?

[jvoung (off chromium), 2014-05-03 00:13:27]

Well I didn't get too far, but here are some comments

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp
File crosstest/test_arith.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp#new...
crosstest/test_arith.cpp:59: // double testFrem(double a, double b) { return a %
b; }
Leave comment for why these are commented out (c/c++ doesn't expose frem)?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode39
src/IceInstX8632.cpp:39: const struct _TypeX8632Attributes {
make indenting consistent here? (BrAttributes were indented 0 spaces and
TypeAttributes 2 spaces)

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode42
src/IceInstX8632.cpp:42: const char *WidthString; // {byte,word,dword,qword} ptr
not sure why ptr -- does byte, word, dword, qword on their own, not describe the
width?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode184
src/IceInstX8632.cpp:184: : InstX8632(Func, InstX8632::Pop, 1, Dest) {}
Could the MaxSrcs be 0 since there's no addSource() ?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode342
src/IceInstX8632.cpp:342: snprintf(buf, llvm::array_lengthof(buf), "add%s",
TypeX8632Attributes[getDest()->getType()].SdSsString);
80 col

clang-format?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode344
src/IceInstX8632.cpp:344: this, Func);
fit on one line?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode419
src/IceInstX8632.cpp:419: bool ShiftHack = true;
Is setting ShiftHack = true for documentation?  I guess the ShiftReg can only be
"cl", or an immediate? Maybe just document as a comment instead?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode420
src/IceInstX8632.cpp:420: bool EmittedSrc1 = false;
should this be "EmittedSrc2" instead? Or can this be simplified to something
like

if(IsRegister) 
  assert that it's cl  and emit
} else {
 emit immediate
}
Str << "\n"

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode423
src/IceInstX8632.cpp:423: if (ShiftReg && ShiftReg->getRegNum() ==
TargetX8632::Reg_ecx) {
can the register be anything other than ecx?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode636
src/IceInstX8632.cpp:636: //bool isDouble = (getSrc(0)->getType() ==
IceType_f64);
can remove commented out code?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode41
src/IceInstX8632.h:41: OperandX8632(OperandTypeX8632 Kind, Type Ty)
nitty-nit: OperandKindX8632 to match the variable name Kind ?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode57
src/IceInstX8632.h:57: uint32_t Shift = 0) {
Should something assert that the shift is reasonable? (0 to 3?)

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode125
src/IceInstX8632.h:125: Adc,
These are overloaded for various types of arguments. E.g., addss can also
represent addsd.

Will there (or is there) a convenient way to tell if the instructions mayStore
or mayLoad (and which operand holds a memory address) to help with SFI?

Or maybe the SFI'ing code will just iterate through the operands and check.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode417
src/IceInstX8632.h:417: class InstX8632Shld : public InstX8632 {
Note, the (old) 32-bit validator did not accept shld, so we had to turn that off
for LLVM
https://code.google.com/p/nativeclient/issues/detail?id=893.  I think the (old)
64-bit validator I think did accept it though.

Should (TODO?) check if the new validator accepts it =)

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode436
src/IceInstX8632.h:436: // Shrd instruction - shift across a pair of operands.
Nit: indent the comments consistently?

E.g., this one is 2 spaces in while the one on line 456 is 0 spaces in.

[JF, 2014-05-04 23:54:58]

https://codereview.chromium.org/265703002/diff/1/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/265703002/diff/1/src/IceCfg.cpp#newcode92
src/IceCfg.cpp:92: // placePhiLoads() must be called before placePhiStores().
Are they always called one after the other? Could they just be merged?

https://codereview.chromium.org/265703002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInst.h#newcode345
src/IceInst.h:345: None // not part of LLVM; used for unconditional branch
Isn't that True?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode39
src/IceInstX8632.cpp:39: const struct _TypeX8632Attributes {
Same thing on reserved name.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode33
src/IceInstX8632.h:33: __Start = Operand::kTarget,
Identifiers starting with a double underscore are reserved for the
implementation. So are those that start with an underscore and a capital letter
:)

Same below.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode77
src/IceInstX8632.h:77: virtual ~OperandX8632Mem() {}
Delete the owned members.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode116
src/IceInstX8632.h:116: virtual ~VariableSplit() {}
Delete Vars.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode125
src/IceInstX8632.h:125: Adc,
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> These are overloaded for various types of arguments. E.g., addss can also
> represent addsd.
> 
> Will there (or is there) a convenient way to tell if the instructions mayStore
> or mayLoad (and which operand holds a memory address) to help with SFI?
> 
> Or maybe the SFI'ing code will just iterate through the operands and check.

I think you'll further want information on how instructions typically dispatch
on the machine, their expected latency, etc... Sounds like capt'n xmacros to the
rescue!

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode281
src/IceInstX8632.h:281: virtual ~InstX8632Br() {}
Delete owned members.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode312
src/IceInstX8632.h:312: template <InstX8632::InstKindX8632 K, bool ShiftHack =
false>
Weird space before template?

https://codereview.chromium.org/265703002/diff/1/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceOperand.h#newcode323
src/IceOperand.h:323: // RegNum is the allocated register, or -1 if it isn't
s/-1/NoRegister/

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:36: LoweringContext() : Node(NULL) {}
Missing dtor.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:67: // instructions come between the two points.
Can't the deleted instruction be recycled as the insertion point?

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:68: //public:
public comment?

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:79: class TargetLowering {
I think using the CRTP here would be better. The point that calls create would
be the only one that needs to know about the actual target, and then all the
virtual calls get resolved at compile time.

See:
  http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern

You can even CRTP on both target and optimization level.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLoweringX8632.h
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLoweringX8632.h...
src/IceTargetLoweringX8632.h:43: return (typeWidthInBytes(Ty) + 3) & ~3;
Can you explain?

[Jim Stichnoth, 2014-05-05 07:03:54]

Latest patchset addresses all of JF's and Jan's comments so far.

https://codereview.chromium.org/265703002/diff/1/Makefile
File Makefile (right):

https://codereview.chromium.org/265703002/diff/1/Makefile#newcode32
Makefile:32: CXXFLAGS := -Wall -Wextra -Werror -fno-rtti -fno-exceptions \
On 2014/05/01 00:16:55, JF wrote:
> That's bold. Kudos if you can keep -Wextra, but I've seen it complain about
> really annoying stuff in the past!

So far so good.  The only complaint to deal with was unused function arguments. 
I tried to crank it up to 11, but that revealed problems in LLVM headers.

https://codereview.chromium.org/265703002/diff/1/Makefile#newcode70
Makefile:70: (cd crosstest; LLVM_BIN_PATH=$(LLVM_BIN_PATH) ./runtests.sh)
On 2014/05/01 00:16:55, JF wrote:
> When will it be appropriate to add test bots to subzero?

Should be doable and appropriate now - as of the previous CL push.

'make check' is all that's needed, but the LLVM toolchain needs to be built and
LLVM_BIN_PATH needs to point to it.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:17: argparser.add_argument('--target', required=False,
default='X8632',
On 2014/05/01 00:16:55, JF wrote:
> X8632 isn't the naming you followed in the previous CL?

Looks like I was inconsistent in the previous CL.

I changed llvm2ice.cpp to allow strings x8632, x86-32, x86_32, x8664, x86-64,
x86_64, arm, arm32, and arm64, to meet the best-in-breed standards of the rest
of the PNaCl toolchain.

(But I limited this script to the choices x8632, x8664, and arm.)

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:20: choices=['m1', '-1', '0', '1', '2'],
On 2014/05/01 00:16:55, JF wrote:
> Are m1 and -1 the same?

Yes.  Added to the help string.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:39: shellcmd('sed -i
"s/le32-unknown-nacl/i686-pc-linux-gnu/" ' + bitcode)
On 2014/05/01 00:16:55, JF wrote:
> sed?

okok, I redid this with python regex.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:50: shellcmd(['$LLVM_BIN_PATH/llvm-mc',
On 2014/05/01 00:16:55, JF wrote:
> Shouldn't LLVM_BIN_PATH be an argument instead?

Done. Added --llvm-bin-path arg, and changed runtests.sh to use it.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:51: '-arch=x86',
On 2014/05/01 00:16:55, JF wrote:
> x86 always?

Added the arch_map dictionary to map Subzero --target strings to llvm-mc --arch
strings.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:62: #objs.append(obj_llc)
On 2014/05/01 00:16:55, JF wrote:
> Commented out.

This was left in intentionally, but I cleaned it up and documented it.  For the
interested/lazy:

# Each original bitcode file needs to be translated by the
# LLVM toolchain and have its object file linked in.  There
# are two ways to do this: explicitly use llc, or include the
# .ll file in the link command.  It turns out that these two
# approaches can produce different semantics on some undefined
# bitcode behavior.  Specifically, LLVM produces different
# results for overflowing fptoui instructions for i32 and i64
# on x86-32.  As it turns out, Subzero lowering was based on
# inspecting the object code produced by the direct llc
# command, so we need to directly run llc on the bitcode, even
# though it makes this script longer, to avoid spurious
# failures.  This behavior can be inspected by switching
# use_llc between True and False.

https://codereview.chromium.org/265703002/diff/1/crosstest/crosstest.py#newco...
crosstest/crosstest.py:66: ['-lm', '-o', os.path.join(args.dir, args.output)])
On 2014/05/01 00:16:55, JF wrote:
> Doesn't this sometimes need to be clang++ to find the C++ libraries?

You are right.  'clang' doesn't work as the linker if the driver uses e.g.
std::cout.  'clang++' seems to compile the driver as C++, which messes up the
external linkage.

It's now changed to select between clang and clang++ based on the driver's
extension.

https://codereview.chromium.org/265703002/diff/1/crosstest/runtests.sh
File crosstest/runtests.sh (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/runtests.sh#newcode2
crosstest/runtests.sh:2: 
On 2014/05/01 00:16:55, JF wrote:
> Comment explaining when to add tests here. Maybe testing should be automated
> through glob?

Actually, this script is a stop-gap until I can wrestle the individual tests
into the lit framework.

Added a TODO explaining this.

https://codereview.chromium.org/265703002/diff/1/crosstest/runtests.sh#newcode3
crosstest/runtests.sh:3: set -x
On 2014/05/01 00:16:55, JF wrote:
> You should also set -e and set -u, and then you can drop the || exit 1 below.

Done.

https://codereview.chromium.org/265703002/diff/1/crosstest/simple_loop.c
File crosstest/simple_loop.c (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/simple_loop.c#newc...
crosstest/simple_loop.c:1: int simple_loop(int *a, int n) {
On 2014/05/01 00:16:55, JF wrote:
> Comment explaining test.

Done.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp
File crosstest/test_arith.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp#new...
crosstest/test_arith.cpp:4: 
On 2014/05/01 00:16:55, JF wrote:
> +1 on templates and macros :D

Done!

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp#new...
crosstest/test_arith.cpp:9: bool testAdd(bool a, bool b) { return a + b; }
On 2014/05/01 00:16:55, JF wrote:
> I'm not sure it's worth testing none-bitwise operations on bool.

I think it's good to keep the weird bool tests as long as they're allowed by the
PNaCl ABI.  At the very least to catch Subzero crashes or things of that nature.
 If we run into problems with e.g. undefined behavior, we can selectively remove
the unit tests.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith.cpp#new...
crosstest/test_arith.cpp:59: // double testFrem(double a, double b) { return a %
b; }
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> Leave comment for why these are commented out (c/c++ doesn't expose frem)?

Done.  TODO added about adding tests for them.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cpp
File crosstest/test_arith_main.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:3: 
On 2014/05/01 00:16:55, JF wrote:
> I think that this test could be partly evaluated at compile-time when built
with
> LTO. You probably want to make the access to Values be volatile, or create a
> no-inline getter to Values, so that the compiler can't cleverly optimize the
> test away.

Done.  I changed the arrays to be volatile (here and elsewhere).

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:10: }
On 2014/05/01 00:16:55, JF wrote:
> Ew. That is quite ugly... and kind of clever. Could you explain it?

Done.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:19: const static unsigned NumValues =
sizeof(Values) / sizeof(*Values);
On 2014/05/01 00:16:55, JF wrote:
> size_t, same on loop bounds.

Done.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:27: bool DisallowIllegal; // for divide related
tests
On 2014/05/01 00:16:55, JF wrote:
> Could you instead figure out that the test is illegal from its name? You
already
> check the value. You're also exercising undefined behavior with the shift by
> larger than / equal to bitwidth.

The name is only meant for printing failing tests, so I don't think it's a good
idea to overload it to affect the testing.

Regarding undefined behavior - if practical, I would prefer Subzero to produce
the same results as llc even when the behavior is undefined, just to minimize
time tracking down "bugs" in broken tests.  But with that said, I'm changing the
name DisallowIllegal to indicate disallowing exceptions rather than arbitrary
undefined behavior.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:49: };
On 2014/05/01 00:16:55, JF wrote:
> xmacros!!! It seems like the test declaration/definition/registration could
all
> be folded together, and it would be more readable and unlikely to diverge that
> way.

Done.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_arith_main.cp...
crosstest/test_arith_main.cpp:216: }
On 2014/05/01 00:16:55, JF wrote:
> It seems like the 3 loops above are pretty much the same thing. Could you
factor
> it out?

Done.  BTW, after refactoring, I discovered that I neglected to add the FP
tests, so this is now added.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.cpp
File crosstest/test_cast.cpp (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.cpp#newc...
crosstest/test_cast.cpp:8: };
On 2014/05/01 00:16:55, JF wrote:
> Couldn't this avoid the class, and just be a templated function?

Done.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.cpp#newc...
crosstest/test_cast.cpp:35: // icmp back to trunc or fptoui.
On 2014/05/01 00:16:55, JF wrote:
> TODO

Actually it looks like I was still in the middle of templatizing this test. 
Should be much better now.

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.h
File crosstest/test_cast.h (right):

https://codereview.chromium.org/265703002/diff/1/crosstest/test_cast.h#newcode13
crosstest/test_cast.h:13: signed char castUi64ToSi8(uint64_t a);
On 2014/05/01 00:16:55, JF wrote:
> Why not use the [u]int_t typedefs here?

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/265703002/diff/1/src/IceCfg.cpp#newcode92
src/IceCfg.cpp:92: // placePhiLoads() must be called before placePhiStores().
On 2014/05/04 23:54:58, JF wrote:
> Are they always called one after the other? Could they just be merged?

I assume you mean a single loop over Nodes, with loop body
"(*I)->placePhiLoads(); (*I)->placePhiStores();"?  This unfortunately doesn't
work because placePhiStores() assumes the phi instruction in the successor basic
block has been rewritten with a temporary in placePhiLoads(), but this hasn't
necessarily happened yet, particularly for backward branches.

https://codereview.chromium.org/265703002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInst.h#newcode345
src/IceInst.h:345: None // not part of LLVM; used for unconditional branch
On 2014/05/04 23:54:58, JF wrote:
> Isn't that True?

Weird, I don't know why this was there. :)  It's gone now.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode39
src/IceInstX8632.cpp:39: const struct _TypeX8632Attributes {
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> make indenting consistent here? (BrAttributes were indented 0 spaces and
> TypeAttributes 2 spaces)

"make format" fail. :(

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode39
src/IceInstX8632.cpp:39: const struct _TypeX8632Attributes {
On 2014/05/04 23:54:58, JF wrote:
> Same thing on reserved name.

I changed all "struct _Foo" to "struct Foo_".

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode42
src/IceInstX8632.cpp:42: const char *WidthString; // {byte,word,dword,qword} ptr
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> not sure why ptr -- does byte, word, dword, qword on their own, not describe
the
> width?

WidthString is only used for the emit() routines.  The Intel-style asm syntax
generally prefixes memory addressing modes with "byte ptr", "dword ptr", etc.,
and the "ptr" part is *always* there, so I just factored it into the string
here.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode184
src/IceInstX8632.cpp:184: : InstX8632(Func, InstX8632::Pop, 1, Dest) {}
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> Could the MaxSrcs be 0 since there's no addSource() ?

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode342
src/IceInstX8632.cpp:342: snprintf(buf, llvm::array_lengthof(buf), "add%s",
TypeX8632Attributes[getDest()->getType()].SdSsString);
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> 80 col
> 
> clang-format?

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode344
src/IceInstX8632.cpp:344: this, Func);
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> fit on one line?

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode419
src/IceInstX8632.cpp:419: bool ShiftHack = true;
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> Is setting ShiftHack = true for documentation?  I guess the ShiftReg can only
be
> "cl", or an immediate? Maybe just document as a comment instead?

Fixed based on your suggestion below.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode420
src/IceInstX8632.cpp:420: bool EmittedSrc1 = false;
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> should this be "EmittedSrc2" instead? Or can this be simplified to something
> like
> 
> if(IsRegister) 
>   assert that it's cl  and emit
> } else {
>  emit immediate
> }
> Str << "\n"

Yeah - I think this was the remnants of copy&paste from emitTwoAddress().  Fixed
here and for shrd.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode423
src/IceInstX8632.cpp:423: if (ShiftReg && ShiftReg->getRegNum() ==
TargetX8632::Reg_ecx) {
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> can the register be anything other than ecx?

No - it's either an immediate or cl.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode636
src/IceInstX8632.cpp:636: //bool isDouble = (getSrc(0)->getType() ==
IceType_f64);
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> can remove commented out code?

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode33
src/IceInstX8632.h:33: __Start = Operand::kTarget,
On 2014/05/04 23:54:58, JF wrote:
> Identifiers starting with a double underscore are reserved for the
> implementation. So are those that start with an underscore and a capital
letter
> :)
> 
> Same below.

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode41
src/IceInstX8632.h:41: OperandX8632(OperandTypeX8632 Kind, Type Ty)
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> nitty-nit: OperandKindX8632 to match the variable name Kind ?

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode57
src/IceInstX8632.h:57: uint32_t Shift = 0) {
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> Should something assert that the shift is reasonable? (0 to 3?)

Done - in the ctor in IceInstX8632.cpp.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode77
src/IceInstX8632.h:77: virtual ~OperandX8632Mem() {}
On 2014/05/04 23:54:58, JF wrote:
> Delete the owned members.

I don't think there are actually any owned members.  Variables are owned
(pooled) at the Cfg level, and Constants are pooled at the GlobalContext level.

If on the other hand OperandX8632Mem held a general Operand or another
OperandX8632Mem, then we would have to do something about explicit ownership.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode116
src/IceInstX8632.h:116: virtual ~VariableSplit() {}
On 2014/05/04 23:54:58, JF wrote:
> Delete Vars.

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode125
src/IceInstX8632.h:125: Adc,
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> These are overloaded for various types of arguments. E.g., addss can also
> represent addsd.
> 
> Will there (or is there) a convenient way to tell if the instructions mayStore
> or mayLoad (and which operand holds a memory address) to help with SFI?
> 
> Or maybe the SFI'ing code will just iterate through the operands and check.

My somewhat vague intention for SFI is to integrate it directly into the
lowering and other transformations.  In particular, I want to avoid the part in
PNaCl/LLVM that undoes address mode optimizations to add x86-64 SFI.

But if that doesn't work out, I agree with JF's suggestion below on adding a
table of relevant properties for the architecture-specific instructions.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode125
src/IceInstX8632.h:125: Adc,
On 2014/05/04 23:54:58, JF wrote:
> On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> > These are overloaded for various types of arguments. E.g., addss can also
> > represent addsd.
> > 
> > Will there (or is there) a convenient way to tell if the instructions
mayStore
> > or mayLoad (and which operand holds a memory address) to help with SFI?
> > 
> > Or maybe the SFI'ing code will just iterate through the operands and check.
> 
> I think you'll further want information on how instructions typically dispatch
> on the machine, their expected latency, etc... Sounds like capt'n xmacros to
the
> rescue!

I doubt we'll make use of those kinds of properties until we get firmly past O0
aspirations.  But by now I'm far enough onto the x-macros train that I'll know
what to do when we get there. :)

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode281
src/IceInstX8632.h:281: virtual ~InstX8632Br() {}
On 2014/05/04 23:54:58, JF wrote:
> Delete owned members.

None are owned by this instruction.

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode312
src/IceInstX8632.h:312: template <InstX8632::InstKindX8632 K, bool ShiftHack =
false>
On 2014/05/04 23:54:58, JF wrote:
> Weird space before template?

OK, I must have forgotten to do a "make format" (which runs clang-format on
everything) before uploading, and emacs led me astray.  (Emacs wants top-level
constructs indented 2 spaces because of the enclosing namespace.)

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode417
src/IceInstX8632.h:417: class InstX8632Shld : public InstX8632 {
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> Note, the (old) 32-bit validator did not accept shld, so we had to turn that
off
> for LLVM
> https://code.google.com/p/nativeclient/issues/detail?id=893.  I think the
(old)
> 64-bit validator I think did accept it though.
> 
> Should (TODO?) check if the new validator accepts it =)

Done, added a TODO.  I assume the same is true for shrd?

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.h#newcode436
src/IceInstX8632.h:436: // Shrd instruction - shift across a pair of operands.
On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> Nit: indent the comments consistently?
> 
> E.g., this one is 2 spaces in while the one on line 456 is 0 spaces in.

Another victim of my "make format" fail. :(

https://codereview.chromium.org/265703002/diff/1/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceOperand.h#newcode323
src/IceOperand.h:323: // RegNum is the allocated register, or -1 if it isn't
On 2014/05/04 23:54:58, JF wrote:
> s/-1/NoRegister/

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:36: LoweringContext() : Node(NULL) {}
On 2014/05/04 23:54:58, JF wrote:
> Missing dtor.

Done.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:67: // instructions come between the two points.
On 2014/05/04 23:54:58, JF wrote:
> Can't the deleted instruction be recycled as the insertion point?

I guess that for debugging codegen, I prefer that the original (deleted)
instruction is shown *before* its expansion.  Using the deleted instruction as
the insert-before point would turn that around.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:68: //public:
On 2014/05/04 23:54:58, JF wrote:
> public comment?

Whoops, gone now.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:79: class TargetLowering {
On 2014/05/04 23:54:58, JF wrote:
> I think using the CRTP here would be better. The point that calls create would
> be the only one that needs to know about the actual target, and then all the
> virtual calls get resolved at compile time.
> 
> See:
>   http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
> 
> You can even CRTP on both target and optimization level.

Good point - I was thinking about CRTP for the target, but didn't consider
optimization level.

I'll leave a TODO for both target and optlevel until I get more up to speed on
using CRTP.

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLoweringX8632.h
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLoweringX8632.h...
src/IceTargetLoweringX8632.h:43: return (typeWidthInBytes(Ty) + 3) & ~3;
On 2014/05/04 23:54:58, JF wrote:
> Can you explain?

Done.

[jvoung (off chromium), 2014-05-15 23:47:33]

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/1/src/IceInstX8632.cpp#newcode42
src/IceInstX8632.cpp:42: const char *WidthString; // {byte,word,dword,qword} ptr
On 2014/05/05 07:03:55, stichnot wrote:
> On 2014/05/03 00:13:28, jvoung (wakacje 5th to 13th) wrote:
> > not sure why ptr -- does byte, word, dword, qword on their own, not describe
> the
> > width?
> 
> WidthString is only used for the emit() routines.  The Intel-style asm syntax
> generally prefixes memory addressing modes with "byte ptr", "dword ptr", etc.,
> and the "ptr" part is *always* there, so I just factored it into the string
> here.

Ah ok, I'm clearly not used to the Intel syntax ;-)

https://codereview.chromium.org/265703002/diff/30001/LOWERING.rst
File LOWERING.rst (right):

https://codereview.chromium.org/265703002/diff/30001/LOWERING.rst#newcode4
LOWERING.rst:4: This document discusses several issues around generating
target-specific ICE
Thanks for adding this!

https://codereview.chromium.org/265703002/diff/30001/LOWERING.rst#newcode152
LOWERING.rst:152: NewInst = InstX8632Mov::create(Func, Reg, Src);
Given the above discussion about "Src->setPreferredRegister(Dst); -- unlikely to
have any effect", does that also hold for this case where Dst is Reg and Reg has
setRegNum(Reg_eax)? Or, does that fall under the "unless ``B`` has been
precolored" category?

https://codereview.chromium.org/265703002/diff/30001/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/265703002/diff/30001/crosstest/crosstest.py#n...
crosstest/crosstest.py:4: import os, sys
one import per line, and sort?

http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Import...

https://codereview.chromium.org/265703002/diff/30001/crosstest/test_arith.def
File crosstest/test_arith.def (right):

https://codereview.chromium.org/265703002/diff/30001/crosstest/test_arith.def...
crosstest/test_arith.def:43: // versions of myFrem() are defined in a seprate
bitcode file.
seprate -> separate

https://codereview.chromium.org/265703002/diff/30001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceCfg.cpp#newcode71
src/IceCfg.cpp:71: dump();
Is the dump() meant to be within the isVerbose() then-block or unconditional?

Well, looks like this is the case in other places too.

https://codereview.chromium.org/265703002/diff/30001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceInstX8632.def#new...
src/IceInstX8632.def:66: X(IceType_f32,  "s", "ss", "dword ptr") \
floating point registers can't be used to address memory (so width doesn't make
sense for them) ?

https://codereview.chromium.org/265703002/diff/30001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceOperand.h#newcode278
src/IceOperand.h:278: // Used primarily for syntactic correctness of textual
assembly
Maybe make the comment stronger about how it is used primarily for syntactic
correctness, since some fields like Weight, RegisterPreference, etc., are not
copied over?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:197: TypeToRegisterSet[IceType_void] =
InvalidRegisters;
Maybe at some point some of this be initialized once and for all instead of for
each Func?

We don't support different calling conventions, so the ScratchRegs (for example)
won't vary across functions, and something decided later like IsEbpBasedFrame
doesn't affect these.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:352: const bool SimpleCoalescing = true;
Doesn't seem like this will ever be set to false.  Is there a plan to add a flag
to control this (or control through the optlevel)?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:442: resetStackAdjustment();
question: when will the StackAdjustment be non-zero at this point (and need
resetStackAdjustment())?  The _push() that are added here have
SuppressStackAdjustment = true.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:523: break;
What are possible instructions at the end of a CFG node that aren't Ret? 
(can't just take the last element of the InstList?)

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:639: llvm::SmallBitVector
TargetX8632::getRegisterSet(RegSetMask Include,
I'm wondering if this needs to be so general. Is this generality more useful
later, when you add the register allocator?

E.g., it seems a bit heavy-weight to call this w/
"getRegisterSet(RegSet_FramePointer, RegSet_None)", when there will only be one
frame pointer?

FWIW, I think LLVM uses a bunch of pre-computed arrays and just chooses a
certain one to return.

Probably okay for now though -- looks like for scratch registers you don't end
up using this function, but have separate ScratchRegs bitvector, so that takes
care of the more common instruction and this is mostly used for prolog/epilog
right now.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:735: Variable *T_4Lo = makeReg(IceType_i32,
Reg_eax);
When is it appropriate to use getPhysicalRegister(Reg_eax) vs makeReg(...,
Reg_eax)? Use makeReg() for a fresh variable when you need to keep the lifetime
accounting separate?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:978: _div(T_ah, Src1, T);
I'm probably missing something:

why is it _div(T_ah, ..., T) then FakeUse(T_ah), instead of _div(T, ..., T_ah)? 
I'm comparing this to the eax/edx for the non-i8 case.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:982: // TODO: fix for 8-bit, see Urem
Should the TODO be under the above branch for == IceType_i8 ?
(and what is broken)

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1611: Func->setError("Phi lowering not
implemented");
nit: "not implemented" sounds like something will be added later -- but Phis
should have been deleted by this point, so this is more of an unexpected
situation? Or is that not the case?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:100: Operand *legalize(Operand *From, LegalMask
Allowed = Legal_All,
Does it make sense to leave a comment for the legalize() method and the related
enum?

I take it this is to avoid having instructions w/out at least one register
operand. Well, there's more to it than that so ... if you think of a short
comment, otherwise might as well look at the code =)

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp#newcode598
src/llvm2ice.cpp:598: clEnumValN(Ice::Opt_0, "O1", "1"),
Should this be Opt_1 for O1?

[Jim Stichnoth, 2014-05-17 14:14:32]

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h
File src/IceTargetLowering.h (right):

https://codereview.chromium.org/265703002/diff/1/src/IceTargetLowering.h#newc...
src/IceTargetLowering.h:79: class TargetLowering {
On 2014/05/05 07:03:55, stichnot wrote:
> On 2014/05/04 23:54:58, JF wrote:
> > I think using the CRTP here would be better. The point that calls create
would
> > be the only one that needs to know about the actual target, and then all the
> > virtual calls get resolved at compile time.
> > 
> > See:
> >   http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern
> > 
> > You can even CRTP on both target and optimization level.
> 
> Good point - I was thinking about CRTP for the target, but didn't consider
> optimization level.
> 
> I'll leave a TODO for both target and optlevel until I get more up to speed on
> using CRTP.

On further thought, I don't think CRTP is appropriate for optlevel, since we may
want different functions to be translated at different opt levels.

https://codereview.chromium.org/265703002/diff/30001/LOWERING.rst
File LOWERING.rst (right):

https://codereview.chromium.org/265703002/diff/30001/LOWERING.rst#newcode152
LOWERING.rst:152: NewInst = InstX8632Mov::create(Func, Reg, Src);
On 2014/05/15 23:47:34, jvoung wrote:
> Given the above discussion about "Src->setPreferredRegister(Dst); -- unlikely
to
> have any effect", does that also hold for this case where Dst is Reg and Reg
has
> setRegNum(Reg_eax)? Or, does that fall under the "unless ``B`` has been
> precolored" category?

When it's time to assign Src (or B) a register, it would find a preference for
Dst (or A) which is precolored with Reg_eax, and thus assign Reg_eax to Src/B as
long as the live ranges don't interfere.

FWIW, in the future I plan to look into making the setPreferredRegister()
mechanism automatic.

I'll assume you aren't asking for a change/clarification in this doc unless you
say otherwise. :)

https://codereview.chromium.org/265703002/diff/30001/crosstest/crosstest.py
File crosstest/crosstest.py (right):

https://codereview.chromium.org/265703002/diff/30001/crosstest/crosstest.py#n...
crosstest/crosstest.py:4: import os, sys
On 2014/05/15 23:47:34, jvoung wrote:
> one import per line, and sort?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Import...

Done.

https://codereview.chromium.org/265703002/diff/30001/crosstest/test_arith.def
File crosstest/test_arith.def (right):

https://codereview.chromium.org/265703002/diff/30001/crosstest/test_arith.def...
crosstest/test_arith.def:43: // versions of myFrem() are defined in a seprate
bitcode file.
On 2014/05/15 23:47:34, jvoung wrote:
> seprate -> separate

Done.

https://codereview.chromium.org/265703002/diff/30001/src/IceCfg.cpp
File src/IceCfg.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceCfg.cpp#newcode71
src/IceCfg.cpp:71: dump();
On 2014/05/15 23:47:34, jvoung wrote:
> Is the dump() meant to be within the isVerbose() then-block or unconditional?
> 
> Well, looks like this is the case in other places too.

It was intentional, but it looks like a good idea to avoid multiple needless
iterations over all the blocks when verbose is off.

Changed both places in this file, plus IceTargetLoweringX8632.cpp.

https://codereview.chromium.org/265703002/diff/30001/src/IceInstX8632.def
File src/IceInstX8632.def (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceInstX8632.def#new...
src/IceInstX8632.def:66: X(IceType_f32,  "s", "ss", "dword ptr") \
On 2014/05/15 23:47:34, jvoung wrote:
> floating point registers can't be used to address memory (so width doesn't
make
> sense for them) ?

This would be used in cases like this:

	movsd	xmm0, qword ptr [esp+12]
	addsd	xmm0, qword ptr [esp+20]
	movsd	qword ptr [esp], xmm0

The type refers to both the register and the stack location, and also for the
memory operand in the Load and Store instructions.

https://codereview.chromium.org/265703002/diff/30001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceOperand.h#newcode278
src/IceOperand.h:278: // Used primarily for syntactic correctness of textual
assembly
On 2014/05/15 23:47:34, jvoung wrote:
> Maybe make the comment stronger about how it is used primarily for syntactic
> correctness, since some fields like Weight, RegisterPreference, etc., are not
> copied over?

Done.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:197: TypeToRegisterSet[IceType_void] =
InvalidRegisters;
On 2014/05/15 23:47:34, jvoung wrote:
> Maybe at some point some of this be initialized once and for all instead of
for
> each Func?
> 
> We don't support different calling conventions, so the ScratchRegs (for
example)
> won't vary across functions, and something decided later like IsEbpBasedFrame
> doesn't affect these.

This is all true.  For now, I'd like to leave a TODO about factoring this out
into some sort of static or pseudo-static initializer.

The problem is that TargetX8632 isn't determined or referenced until
TargetLowering::createLowering() is called during the Cfg constructor.  Probably
some sort of CRTP-like magic would be appropriate here.  Alternatively, do a
locked first-time initialization in every TargetX8632 ctor, but I'd prefer to
keep locking contained within GlobalContext.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:352: const bool SimpleCoalescing = true;
On 2014/05/15 23:47:34, jvoung wrote:
> Doesn't seem like this will ever be set to false.  Is there a plan to add a
flag
> to control this (or control through the optlevel)?

I didn't plan to expose this by a flag, since SimpleCoalescing is pretty much
always a win.  The intention is to keep it internal just for benchmarking.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:442: resetStackAdjustment();
On 2014/05/15 23:47:34, jvoung wrote:
> question: when will the StackAdjustment be non-zero at this point (and need
> resetStackAdjustment())?  The _push() that are added here have
> SuppressStackAdjustment = true.

Currently it should always be zero at this point.  This explicit reset is just
in case _sub() gets clever enough in the future to update the adjustment, such
as in the preceding statement.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:523: break;
On 2014/05/15 23:47:34, jvoung wrote:
> What are possible instructions at the end of a CFG node that aren't Ret? 
> (can't just take the last element of the InstList?)

TargetX8632::lowerRet() adds a FakeUse of esp at the end, to make sure the
various esp frame adjustments don't get eliminated.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:639: llvm::SmallBitVector
TargetX8632::getRegisterSet(RegSetMask Include,
On 2014/05/15 23:47:34, jvoung wrote:
> I'm wondering if this needs to be so general. Is this generality more useful
> later, when you add the register allocator?
> 
> E.g., it seems a bit heavy-weight to call this w/
> "getRegisterSet(RegSet_FramePointer, RegSet_None)", when there will only be
one
> frame pointer?
> 
> FWIW, I think LLVM uses a bunch of pre-computed arrays and just chooses a
> certain one to return.
> 
> Probably okay for now though -- looks like for scratch registers you don't end
> up using this function, but have separate ScratchRegs bitvector, so that takes
> care of the more common instruction and this is mostly used for prolog/epilog
> right now.

Yes, that instance of getRegisterSet() is kind of silly, but it's inside an
assert()...

For register allocation, it sets up CallerSave|CalleeSave as the Include set,
and StackPointer (plus FramePointer if appropriate) as the Exclude set.  I think
this whole thing can be reevaluated after implementing for x86-64 and ARM.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:735: Variable *T_4Lo = makeReg(IceType_i32,
Reg_eax);
On 2014/05/15 23:47:34, jvoung wrote:
> When is it appropriate to use getPhysicalRegister(Reg_eax) vs makeReg(...,
> Reg_eax)? Use makeReg() for a fresh variable when you need to keep the
lifetime
> accounting separate?

That's right.  getPhysicalRegister() is used for things that don't really need
their lifetime tracked: pushing callee-save registers in the prolog, reference
to stack/frame pointer, and killing scratch registers at call sites.  Using a
single instance of a Variable keeps the number of Variables under control.

For variables with normal lifetime tracking, the liveness analysis (to come
later) assumes that the live range begins and ends at most once per basic block,
so using getPhysicalRegister for different purposes in the same basic block
could prevent that register from being allocated to another purpose during the
interval where it is really free.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:978: _div(T_ah, Src1, T);
On 2014/05/15 23:47:34, jvoung wrote:
> I'm probably missing something:
> 
> why is it _div(T_ah, ..., T) then FakeUse(T_ah), instead of _div(T, ...,
T_ah)? 
> I'm comparing this to the eax/edx for the non-i8 case.

Nice - your simpler suggestion seems to work. :)

Probably the original sequence came from adapting the Urem i8 implementation,
rather than from the Udiv non-i8 implementation.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:982: // TODO: fix for 8-bit, see Urem
On 2014/05/15 23:47:34, jvoung wrote:
> Should the TODO be under the above branch for == IceType_i8 ?
> (and what is broken)

Done.  I think that TODO was left in by accident.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1611: Func->setError("Phi lowering not
implemented");
On 2014/05/15 23:47:34, jvoung wrote:
> nit: "not implemented" sounds like something will be added later -- but Phis
> should have been deleted by this point, so this is more of an unexpected
> situation? Or is that not the case?

My longer-term plan is to delay phi lowering until after register allocation. 
As such, phis could technically still be part of the IR at this point, but even
if so, they should be segregated into CfgNode::Phis and not part of
CfgNode::Insts.

In any case, I made the error message more precise.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.h:100: Operand *legalize(Operand *From, LegalMask
Allowed = Legal_All,
On 2014/05/15 23:47:34, jvoung wrote:
> Does it make sense to leave a comment for the legalize() method and the
related
> enum?
> 
> I take it this is to avoid having instructions w/out at least one register
> operand. Well, there's more to it than that so ... if you think of a short
> comment, otherwise might as well look at the code =)

Yeah, that's a good idea.  I left a comment here, and some more in the source
code.

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp#newcode598
src/llvm2ice.cpp:598: clEnumValN(Ice::Opt_0, "O1", "1"),
On 2014/05/15 23:47:34, jvoung wrote:
> Should this be Opt_1 for O1?

Done.

[jvoung (off chromium), 2014-05-19 20:28:54]

whew -- looked through the rest of the TargetLowerX8632 file

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1055: Operand *Src0 = legalize(Inst->getSrc(0));
Probably on your mind already since you noted a TODO under mul for
strength-reducing when some operands are constants:

Might be worth TODO checking if Dest is a reg and src is 0 and the flags can be
clobbered, and doing xor reg, reg instead -- if worth the translation time
overhead of checking for that pattern.

There are also some cases elsewhere w/ "_mov(T_i, Zero)", but I guess you don't
know if T_i should be in a register or not.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1070: Operand *RI = legalize(Src0, Legal_Reg |
Legal_Imm, AllowOverlap);
Does this mean that Inst->getSrc(0) gets legalized twice (once here w/ stricter
mask and once at line 1055 w/ a looser mask)?

Would it work to only legalize once (move the other legalize into the if
(Dest->getType() == IceType_i64) branch?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1081: _cmp(Src0, Zero);
Is it better to _test reg,reg than _cmp reg, zero? I guess you don't know if Src
will be in a register or not?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1164: if (!KilledRegs.empty()) {
For x86 this is always true because there's always some ScratchReg? Well, it's a
small point so up to you for removing the check (or turning it into an assert).

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1170: if (Instr->hasSideEffects() && eax) {
for InstrCall, hasSideEffects() is always true?

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1319: // Possibilities are cvtftoui32,
cvtdtoui32, cvtftoui64, cvtdtoui64
For the cases where gcc would have just invoked a libgcc method anyway (like for
the case of cvtdtoui64), were you planning on later calling the compiler-rt
version directly (e.g., __fixunsdfdi)?

Was the problem that the pnacl driver complained about undefined symbols when
referring to compiler-rt functions?

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp#newcode689
src/llvm2ice.cpp:689: errs() << "ICE translation error: " << Func->getError() <<
"\n";
Should this also toggle a flag that can help decide on an exit status other than
0?

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1493: // The table is indexed by
InstFcmp::Condition.  Make sure it didn't fall
Should have already been checked by xMacroIntegrityCheck ?

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1536: bool IsSrc1ImmOrReg = false;
Can move this to below the i64 case, since it's not used in the i64 case?

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1581: // cmp b, c
Similarly, would it be worth TODO use _test when one operand is constant 0 and
the other is a reg?

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1658: _cmp(Condition, Zero);
Another place w/ _test vs _cmp

[Jim Stichnoth, 2014-05-20 18:20:07]

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1055: Operand *Src0 = legalize(Inst->getSrc(0));
On 2014/05/19 20:28:54, jvoung wrote:
> Probably on your mind already since you noted a TODO under mul for
> strength-reducing when some operands are constants:
> 
> Might be worth TODO checking if Dest is a reg and src is 0 and the flags can
be
> clobbered, and doing xor reg, reg instead -- if worth the translation time
> overhead of checking for that pattern.
> 
> There are also some cases elsewhere w/ "_mov(T_i, Zero)", but I guess you
don't
> know if T_i should be in a register or not.

My thought on these kinds of opportunities (e.g. setting a register to 0, or
comparing a register to 0, or other common idioms with shorter encodings or some
other advantage) is to handle them either as a post register allocation peephole
optimization pass, or to put the peephole logic directly into the emitter.

As you point out, we would need to make sure the flags are affected in the
expected way.  For example, the icmp/fcmp/select lowering is sometimes like
this:

  cmp src1, src2
  mov dest, 0
  jle label
  mov dest, 1
label:

We could swap the first two instructions, but that would guarantee that dest's
live range interferes with src1 and src2, increasing register pressure.  So with
this lowering, we would want to forbid the first mov from affecting flags.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1070: Operand *RI = legalize(Src0, Legal_Reg |
Legal_Imm, AllowOverlap);
On 2014/05/19 20:28:54, jvoung wrote:
> Does this mean that Inst->getSrc(0) gets legalized twice (once here w/
stricter
> mask and once at line 1055 w/ a looser mask)?
> 
> Would it work to only legalize once (move the other legalize into the if
> (Dest->getType() == IceType_i64) branch?

Done.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1081: _cmp(Src0, Zero);
On 2014/05/19 20:28:54, jvoung wrote:
> Is it better to _test reg,reg than _cmp reg, zero? I guess you don't know if
Src
> will be in a register or not?

That's right.  That could be done as a peephole as described above.  I believe
the test instruction affects flags differently than comparing to 0, but unlikely
in a way we care about.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1164: if (!KilledRegs.empty()) {
On 2014/05/19 20:28:54, jvoung wrote:
> For x86 this is always true because there's always some ScratchReg? Well, it's
a
> small point so up to you for removing the check (or turning it into an
assert).

A bit of pedantry, I guess, in case we ever try out an ABI that entirely lacks
scratch registers. :)  Even then, it's just a minor optimization since a
FakeKill instructions with an empty variable list will just be a no-op.

I've removed the condition.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1170: if (Instr->hasSideEffects() && eax) {
On 2014/05/19 20:28:54, jvoung wrote:
> for InstrCall, hasSideEffects() is always true?

Today it's true, but I'm looking ahead to allowing dead-code elimination to
eliminate intrinsic calls or other calls that are somehow known to lack side
effect.

https://codereview.chromium.org/265703002/diff/30001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1319: // Possibilities are cvtftoui32,
cvtdtoui32, cvtftoui64, cvtdtoui64
On 2014/05/19 20:28:54, jvoung wrote:
> For the cases where gcc would have just invoked a libgcc method anyway (like
for
> the case of cvtdtoui64), were you planning on later calling the compiler-rt
> version directly (e.g., __fixunsdfdi)?
> 
> Was the problem that the pnacl driver complained about undefined symbols when
> referring to compiler-rt functions?

Actually, these helper function names may resemble the gcc versions, but you'll
see in crosstest/test_cast_main.cpp that I have provided actual implementations
(this is the only place where we actually attempt to link Subzero-produced
code).

Adding a TODO to the several places that create calls to these cvt* functions.

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/265703002/diff/30001/src/llvm2ice.cpp#newcode689
src/llvm2ice.cpp:689: errs() << "ICE translation error: " << Func->getError() <<
"\n";
On 2014/05/19 20:28:54, jvoung wrote:
> Should this also toggle a flag that can help decide on an exit status other
than
> 0?

Done.

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1493: // The table is indexed by
InstFcmp::Condition.  Make sure it didn't fall
On 2014/05/19 20:28:54, jvoung wrote:
> Should have already been checked by xMacroIntegrityCheck ?

Ah, that comment is for an assert that used to be there before
xMacroIntegrityCheck was devised.  Removed.

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1536: bool IsSrc1ImmOrReg = false;
On 2014/05/19 20:28:54, jvoung wrote:
> Can move this to below the i64 case, since it's not used in the i64 case?

Done.

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1581: // cmp b, c
On 2014/05/19 20:28:54, jvoung wrote:
> Similarly, would it be worth TODO use _test when one operand is constant 0 and
> the other is a reg?

I think this is better as a later peephole, since if Src1 is known to be a
constant 0, then Src0New will have basically unrestricted legalization, and it's
rare that we would know at this point that Src0New is a physical register.

https://codereview.chromium.org/265703002/diff/40001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1658: _cmp(Condition, Zero);
On 2014/05/19 20:28:54, jvoung wrote:
> Another place w/ _test vs _cmp

Similar situation where it's unlikely that we can know that Condition is in a
physical register at this point.

[jvoung (off chromium), 2014-05-22 18:02:37]

LGTM

https://codereview.chromium.org/265703002/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1538: // The table is indexed by
InstIcmp::Condition.  Make sure it didn't fall
Also from before a wild xMacroIntegrityCheck appeared?

[Jim Stichnoth, 2014-05-22 20:19:15]

All comments are addressed.  I have also merged in Karl's previous CL and fixed
any resulting breakage in my code and tests.  I'm planning to push shortly.

https://codereview.chromium.org/265703002/diff/60001/src/IceTargetLoweringX86...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/265703002/diff/60001/src/IceTargetLoweringX86...
src/IceTargetLoweringX8632.cpp:1538: // The table is indexed by
InstIcmp::Condition.  Make sure it didn't fall
On 2014/05/22 18:02:38, jvoung wrote:
> Also from before a wild xMacroIntegrityCheck appeared?

Humph.  I thought I had searched for more instances of this.

[Jim Stichnoth, 2014-05-22 20:38:52]

Committed patchset #6 manually as r5bc2b1d (presubmit successful).