Move X8632-specific Assembler stuff to Machine Traits.

Move X8632-specific Assembler stuff to Machine Traits.

As part of the refactoring moves the MachineTraits<TargetX8632> to a separate header.


BUG= https://code.google.com/p/nativeclient/issues/detail?id=4077
R=jvoung@chromium.org

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

[John, 2015-06-30 20:18:03]

jpp@chromium.org changed reviewers:
+ jvoung@chromium.org, kschimpf@google.com, stichnot@chromium.org

[John, 2015-06-30 20:18:04]

Pretty mechanical (and boring) stuff. But we're closer to X8664. :)

[jvoung (off chromium), 2015-06-30 21:19:48]

Cool -- just a few nits from me (are we still adding the license bits?)

https://codereview.chromium.org/1216033004/diff/80001/src/IceAssemblerX86Base...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceAssemblerX86Base...
src/IceAssemblerX86BaseImpl.h:262: emitUint8(0x0F);
So for x86-64, there would be a trait or something to emit prefixes (e.g.,
EmitOperandREX vs no prefixes)?

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:23: class RegX8632 {
Should RegX8664 get the same treatment (namespace -> class)?

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:4: //
Add boilerplate?

// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:4: //
Add boilerplate?

// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:50: static const GPRRegister Counter =
RegX8632::Encoded_Reg_ecx;
CounterReg? Not sure if Counter is likely to have name clashes.

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:262: // TODO(jpp): move the helper methods below
to the MachineTraits.
So the idea is that this file shouldn't say, for instance, InstX8632XYZ or?

[John, 2015-06-30 21:42:00]

Please take another look.

https://codereview.chromium.org/1216033004/diff/80001/src/IceAssemblerX86Base...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceAssemblerX86Base...
src/IceAssemblerX86BaseImpl.h:262: emitUint8(0x0F);
On 2015/06/30 21:19:48, jvoung wrote:
> So for x86-64, there would be a trait or something to emit prefixes (e.g.,
> EmitOperandREX vs no prefixes)?

Exactly. I am thinking of adding a

MachineTraits::addPrefixes(Type Ty, const Traits::Operand &Operand);

to the specific Traits to handle all instruction prefixes (currently, operand
size override, and REX)

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:23: class RegX8632 {
On 2015/06/30 21:19:48, jvoung wrote:
> Should RegX8664 get the same treatment (namespace -> class)?

This is already part of this patch. Nice catch, though. :)

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:4: //
On 2015/06/30 21:19:48, jvoung wrote:
> Add boilerplate?
> 
> // This file is distributed under the University of Illinois Open Source
> // License. See LICENSE.TXT for details.

Done.

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:4: //
On 2015/06/30 21:19:48, jvoung wrote:
> Add boilerplate?
> 
> // This file is distributed under the University of Illinois Open Source
> // License. See LICENSE.TXT for details.

Done.

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632Traits.h:50: static const GPRRegister Counter =
RegX8632::Encoded_Reg_ecx;
On 2015/06/30 21:19:48, jvoung wrote:
> CounterReg? Not sure if Counter is likely to have name clashes.

I thought about that, but I figured that, since every access to this Counter
will, necessarily, "go through" the type, e.g.,

Traits::Counter.

But you do have a point, "Counter" is too generic. Too keep things "consistent"
I went with

Encoded_Reg_Counter (and Encoded_Reg_Accumulator)

Please let me know what you think.

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86Base.h:262: // TODO(jpp): move the helper methods below
to the MachineTraits.
On 2015/06/30 21:19:48, jvoung wrote:
> So the idea is that this file shouldn't say, for instance, InstX8632XYZ or?

Yes, that's correct. My plan is to have something like

template <class Type, class... Args> Type* createInst(Args&&... args) {
  return TraitsMagic<Type>::create(std::forward<Args>(args)...);
}

so that we can isolate common x86 stuff (add, sub, sbb, etc) from x8632
(cmpxchg8b, fld, fstp, etc).

[jvoung (off chromium), 2015-06-30 22:26:46]

lgtm

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:23: class RegX8632 {
On 2015/06/30 21:42:00, John wrote:
> On 2015/06/30 21:19:48, jvoung wrote:
> > Should RegX8664 get the same treatment (namespace -> class)?
> 
> This is already part of this patch. Nice catch, though. :)

I see "ConditionCodesX8664.h", but not RegistersX8664.h, but I could be missing
something.

[John, 2015-06-30 22:29:25]

Committed patchset #7 (id:120001) manually as
5d0acff3a2fa421923392aadb4df2742064b6248 (presubmit successful).

[John, 2015-06-30 22:29:29]

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h
File src/IceRegistersX8632.h (right):

https://codereview.chromium.org/1216033004/diff/80001/src/IceRegistersX8632.h...
src/IceRegistersX8632.h:23: class RegX8632 {
On 2015/06/30 22:26:46, jvoung wrote:
> On 2015/06/30 21:42:00, John wrote:
> > On 2015/06/30 21:19:48, jvoung wrote:
> > > Should RegX8664 get the same treatment (namespace -> class)?
> > 
> > This is already part of this patch. Nice catch, though. :)
> 
> I see "ConditionCodesX8664.h", but not RegistersX8664.h, but I could be
missing
> something.

I got confused.... Good catch.