Start adding an integrated assembler.

Start adding an integrated assembler.

Patchset 1 is more or less the original dart assembler code (revision 39313)
The other patches show the modifications.

Handling:
* adc, add, sbb, sub
* and, or, xor
* addss/sd, divss/sd, mulss/sd, subss/sd
* addps, divps, mulps, subps
* padd, psub
* sqrtss/sd
* cbw, cwd, cdq
* cmov
* cmpps
* cmp
* ucomiss/sd
* movsxb/w
* movzxb/w
* nop
* pop
* ret

Sort of handling:
* Call (can't force PC-rel reloc w/ llvm-mc)

Jumps need to be handled last because they require
accurate instruction distances.

BUG=xxx

[jvoung (off chromium), 2014-08-26 00:58:08]

Patchset #8 (id:140001) has been deleted

[jvoung (off chromium), 2014-08-26 00:58:12]

Patchset #7 (id:120001) has been deleted

[jvoung (off chromium), 2014-09-04 00:20:28]

Patchset #2 (id:20001) has been deleted

[jvoung (off chromium), 2014-09-04 00:20:36]

Patchset #2 (id:40001) has been deleted

[jvoung (off chromium), 2014-09-04 00:20:44]

Patchset #3 (id:80001) has been deleted

[jvoung (off chromium), 2014-09-04 00:20:53]

Patchset #4 (id:160001) has been deleted

[jvoung (off chromium), 2014-09-04 00:21:01]

Patchset #4 (id:180001) has been deleted

[jvoung (off chromium), 2014-09-04 15:25:13]

Patchset #11 (id:340001) has been deleted

[jvoung (off chromium), 2014-09-04 18:05:04]

Patchset #8 (id:280001) has been deleted

[jvoung (off chromium), 2014-09-04 18:05:15]

Patchset #8 (id:300001) has been deleted

[jvoung (off chromium), 2014-09-04 18:05:23]

Patchset #8 (id:320001) has been deleted

[jvoung (off chromium), 2014-09-04 18:11:07]

jvoung@chromium.org changed reviewers:
+ stichnot@chromium.org

[jvoung (off chromium), 2014-09-04 18:16:56]

Thought I would send out the WIP patch, in case there's something to watch out
for.

This currently handles the easy cases. Instructions w/ XMM regs can be all
handled by one common dispatcher. That could be simplified further if we split
the Assembler-level methods from something like addss()/addsd into
addss_opcodes()/addsd_opcodes; EmitOperand(...). And we'd diverge more and make
the EmitOperand(), etc. methods public instead of private. With that, there is
no need to pass along method pointers because we can just test the type, emit
the opcodes, then jump to the dispatcher to emit the operands w/out care of the
type.

I'll try to handle some more of the integer ones which are not so nice. After
that, would that be a good place to start for a first patch?

[Jim Stichnoth, 2014-09-04 20:57:48]

I haven't looked deeply at the Dart-based assembler code, but here are some
initial comments.

https://codereview.chromium.org/476323004/diff/400001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/476323004/diff/400001/src/IceUtils.h#newcode21
src/IceUtils.h:21: const uint32_t kBitsPerByte = 8;
C++ CHAR_BIT for translator-time uses, and we also have X86_CHAR_BIT for runtime
uses.

https://codereview.chromium.org/476323004/diff/400001/src/IceUtils.h#newcode23
src/IceUtils.h:23: const uint32_t kWordSize = sizeof(uint32_t);
Make it clear whether this is translator-time or runtime usage?  Though it's
hard to imagine the sizes being different.

https://codereview.chromium.org/476323004/diff/400001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/476323004/diff/400001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:112: DisableIntegratedAssembler("no-integrated-as",
Can the sense of this be reversed for better clarity?  E.g.

static cl::opt<bool> UseIntegratedAssembler("integrated-as", cl::init(true));
...
Flags.UseIntegratedAssembler = UseIntegratedAssembler;

[jvoung (off chromium), 2014-09-08 04:38:25]

Patchset #12 (id:440001) has been deleted

[jvoung (off chromium), 2014-09-08 21:39:01]

Thanks, added adc,add,sub,sbb,and,or,xor,cmp,ucomiss.

Might be able to change the adc,add,etc. cases to separate the opcodes from the
operands and avoid some more method pointer-passing.

https://codereview.chromium.org/476323004/diff/400001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/476323004/diff/400001/src/IceUtils.h#newcode21
src/IceUtils.h:21: const uint32_t kBitsPerByte = 8;
On 2014/09/04 20:57:48, stichnot wrote:
> C++ CHAR_BIT for translator-time uses, and we also have X86_CHAR_BIT for
runtime
> uses.

Done. I removed one use that wasn't used, and adjusted some asserts.

In this case, I think it's translator side since it's used to multiply a
translator-side sizeof(T).

https://codereview.chromium.org/476323004/diff/400001/src/IceUtils.h#newcode23
src/IceUtils.h:23: const uint32_t kWordSize = sizeof(uint32_t);
On 2014/09/04 20:57:48, stichnot wrote:
> Make it clear whether this is translator-time or runtime usage?  Though it's
> hard to imagine the sizes being different.

Hmm, I need to look into this more. Maybe this shouldn't be in the general
IceUtils.h.  It's actually sizeof(intptr_t) originally, so it would have been
different for x86-64.

For the x86-32 assembler, it is only used for Label positions (for some reason,
offset them by kWordSize which is 4 or 8). I'm not sure exactly why this is used
to offset Label positions. It's not like it is based on instruction size or size
of disp32 or anything (since it's either 4 or 8).

For the x86-64 assembler, it's also used to represent the size of a 64-bit
register...

I'd like to leave a TODO until I start assembling jumps. It seems like we should
have different constants for the different cases. E.g., we don't want to
conflate size of 64-bit register w/ size of pointer.

https://codereview.chromium.org/476323004/diff/400001/src/llvm2ice.cpp
File src/llvm2ice.cpp (right):

https://codereview.chromium.org/476323004/diff/400001/src/llvm2ice.cpp#newcod...
src/llvm2ice.cpp:112: DisableIntegratedAssembler("no-integrated-as",
On 2014/09/04 20:57:48, stichnot wrote:
> Can the sense of this be reversed for better clarity?  E.g.
> 
> static cl::opt<bool> UseIntegratedAssembler("integrated-as", cl::init(true));
> ...
> Flags.UseIntegratedAssembler = UseIntegratedAssembler;

Done.

I started with UseIntegratedAssembler cl::init(false), but that made testing
more difficult, since I needed to remember exactly which tests get affected and
add the flag to test.

[Jim Stichnoth, 2014-09-09 14:58:42]

https://codereview.chromium.org/476323004/diff/580001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/476323004/diff/580001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:89: // Should we just make a ConstantInteger32?
I was also thinking that maybe constant integers need to be enhanced somewhat. 
At a minimum, adding a flag indicating whether it is intended to be signed or
unsigned (or unknown / don't care?) so that when getting its value, validation
against width can be done.

https://codereview.chromium.org/476323004/diff/580001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/476323004/diff/580001/src/IceUtils.h#newcode22
src/IceUtils.h:22: const uint32_t kWordSize = sizeof(intptr_t);
It's hard to search the patchsets for uses of kWordSize, but this sounds like a
sizeof that should be for the target machine, not necessarily the machine
building llvm2ice.

https://codereview.chromium.org/476323004/diff/580001/src/IceUtils.h#newcode42
src/IceUtils.h:42: (static_cast<unsigned int>(N) < (CHAR_BIT * sizeof(value))));
Same comment as above - is this CHAR_BIT, or the target-specific X86_CHAR_BIT? 
(not that they should ever be different, but...)

[jvoung (off chromium), 2014-09-15 17:19:15]

https://codereview.chromium.org/476323004/diff/580001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/476323004/diff/580001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:89: // Should we just make a ConstantInteger32?
On 2014/09/09 14:58:41, stichnot wrote:
> I was also thinking that maybe constant integers need to be enhanced somewhat.

> At a minimum, adding a flag indicating whether it is intended to be signed or
> unsigned (or unknown / don't care?) so that when getting its value, validation
> against width can be done.

Hmm, we could add signed/unsigned, but I'm not yet sure how that would be used.
Constants read in from the bitcode file are always sign-extended to the constant
pool's storage type. Constants created from within subzero might be sign or zero
extended. E.g., what happens when adding two constants in the addressing mode
optimization and making a new constant.

Regardless, it seemed like if we had a 32-bit constant pool as well, then the
check here could be free. Lots of constants that we use internally are 32-bit or
less. Constants read in by the bitcode reader could be classified at the time
it's read, and by the time we emit instructions/bytes, the lowering should have
split 64-bit constants into 32-bit ones. Maybe that's a bit too x86 specific...
For ARM, lowering would split a constant into upper16 and lower16 instead, or it
might want to know that a constant offset fits in 12-bits for load/store.

Anyway, I tried splitting the constant pool more, to see what that would look
like:
https://codereview.chromium.org/569033002/

Does that seem worthwhile?

https://codereview.chromium.org/476323004/diff/580001/src/IceUtils.h
File src/IceUtils.h (right):

https://codereview.chromium.org/476323004/diff/580001/src/IceUtils.h#newcode22
src/IceUtils.h:22: const uint32_t kWordSize = sizeof(intptr_t);
On 2014/09/09 14:58:41, stichnot wrote:
> It's hard to search the patchsets for uses of kWordSize, but this sounds like
a
> sizeof that should be for the target machine, not necessarily the machine
> building llvm2ice.

I moved this to the one place it's being used, which is now in a target specific
file. I still need to look into why this is being used that way. There are
separate places where "+/- 4" and "+/- 1" are hard coded (imm32, imm8), while
there is also this kWordSize separate from the "4".

https://codereview.chromium.org/476323004/diff/580001/src/IceUtils.h#newcode42
src/IceUtils.h:42: (static_cast<unsigned int>(N) < (CHAR_BIT * sizeof(value))));
On 2014/09/09 14:58:42, stichnot wrote:
> Same comment as above - is this CHAR_BIT, or the target-specific X86_CHAR_BIT?

> (not that they should ever be different, but...)

I think this is plain CHAR_BIT, since it's used along with sizeof(T) (where T is
some type specified by the subzero code).

Since sizeof(char) is always 1, and we're trying to figure out the number of
bits, I think the multiplier should be CHAR_BIT.

[jvoung (off chromium), 2014-09-16 00:46:02]

Patchset #22 (id:660001) has been deleted

[jvoung (off chromium), 2014-09-16 22:41:15]

I'm going to close this CL and split it up...