Handle "Mov" which is mov, movss, movsd, and used for nacl.read.tp.

Handle "Mov" which is mov, movss, movsd, and used for nacl.read.tp.

Currently, this only checks and emits the segment
override only for GPR instructions, assuming it's mostly
only used for nacl.read.tp. The code will assert when used
in other situations.

The lea hack is still tested in some files, but it's not
emitted with emitIAS, and instead the "immediate" operand
now has a fixup.

There is a more compact encoding for "mov eax, moffs32",
etc., but that isn't used right now.

BUG=none
R=stichnot@chromium.org

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

[jvoung (off chromium), 2014-10-09 23:54:04]

Patchset #3 (id:170001) has been deleted

[jvoung (off chromium), 2014-10-10 00:54:42]

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

[jvoung (off chromium), 2014-10-10 00:55:50]

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

https://codereview.chromium.org/649463002/diff/1/src/IceOperand.h#newcode23
src/IceOperand.h:23: #include "IceGlobalContext.h"
From the include "IceOperand.h" in assembler_ia32.h. Since this file does:
Func->getContext()->getStrDump();

https://codereview.chromium.org/649463002/diff/190001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (left):

https://codereview.chromium.org/649463002/diff/190001/src/IceInstX8632.cpp#ol...
src/IceInstX8632.cpp:2486: assert(CR->getOffset() == 0);
This is now tested, since 64-bit Variable splitting can make Offset += 4

https://codereview.chromium.org/649463002/diff/190001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/convert.ll (right):

https://codereview.chromium.org/649463002/diff/190001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/convert.ll:47: ; i64v and i64+4.
I'll see if this is fixed by adding Store::emitIAS()

[Jim Stichnoth, 2014-10-10 17:37:25]

https://codereview.chromium.org/649463002/diff/240001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:23: #include "IceClFlags.h"
I'm curious why this is needed?  (Also saw the same new include in Karl's CL.)

https://codereview.chromium.org/649463002/diff/240001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:32: class ClFlags;
Remove this in light of the new include?

https://codereview.chromium.org/649463002/diff/240001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:2587: Offset += 4;
Maybe we should change this "4" to something more symbolic?  E.g.

int32_t VariableSplit::getOffset() const { return Part == High ? 4 : 0; }

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h#newcode21
src/IceOperand.h:21: #include "IceCfg.h"
Why are these includes needed?

In general, I prefer to limit includes in header files to IceDefs.h and
IceTypes.h, if possible, and add includes to the .cpp files.

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h#newcod...
src/IceOperand.h:212: // TODO(jvoung): offset should only be int32_t, since we
are ILP32.
I wonder if this should just be replace now with int32_t (if practical), and fix
it later to be more general if ever needed?

https://codereview.chromium.org/649463002/diff/240001/src/assembler_ia32.cpp
File src/assembler_ia32.cpp (left):

https://codereview.chromium.org/649463002/diff/240001/src/assembler_ia32.cpp#...
src/assembler_ia32.cpp:28: const Type BrokenType = IceType_i32;
Yay!

[jvoung (off chromium), 2014-10-10 21:03:39]

https://codereview.chromium.org/649463002/diff/240001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:23: #include "IceClFlags.h"
On 2014/10/10 17:37:25, stichnot wrote:
> I'm curious why this is needed?  (Also saw the same new include in Karl's CL.)

Hmm, doesn't seem to be needed for me anymore, and I don't remember what the
problem was exactly. Unlike the other case, this file seems to only need the fwd
decl.

Removing

https://codereview.chromium.org/649463002/diff/240001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:32: class ClFlags;
On 2014/10/10 17:37:25, stichnot wrote:
> Remove this in light of the new include?

Acknowledged.

https://codereview.chromium.org/649463002/diff/240001/src/IceInstX8632.cpp
File src/IceInstX8632.cpp (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceInstX8632.cpp#ne...
src/IceInstX8632.cpp:2587: Offset += 4;
On 2014/10/10 17:37:25, stichnot wrote:
> Maybe we should change this "4" to something more symbolic?  E.g.
> 
> int32_t VariableSplit::getOffset() const { return Part == High ? 4 : 0; }

Sounds good -- Done.

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h#newcode21
src/IceOperand.h:21: #include "IceCfg.h"
On 2014/10/10 17:37:25, stichnot wrote:
> Why are these includes needed?
> 
> In general, I prefer to limit includes in header files to IceDefs.h and
> IceTypes.h, if possible, and add includes to the .cpp files.

For this, I added it because the .h file itself does Func->getContext(), and
Func->allocate(), and therefore depends on the definition of the Cfg.


Otherwise, futher down the line, if assembler_ia32.h only fwd declares stuff
from IceOperand.h instead of including it I get:

In file included from src/assembler_ia32.cpp:20:
src/assembler_ia32.h:76:30: error: member access into incomplete type 'const
Ice::ConstantRelocatable'
      : value_(fixup->value()->getOffset()), fixup_(fixup) {
                             ^
I can re-order the includes in assembler_ia32.cpp to have IceOperand.h first,
then assembler_ia32.h later, but then that has a different issue.

If I include IceOperand.h in assembler_ia32.h, then I get:

In file included from src/assembler_ia32.cpp:20:
In file included from src/assembler_ia32.h:25:
src/IceOperand.h:64:20: error: member access into incomplete type 'const
Ice::Cfg'
    dump(Func, Func->getContext()->getStrDump());
                   ^
src/IceDefs.h:39:7: note: forward declaration of 'Ice::Cfg'
class Cfg;
      ^
In file included from src/assembler_ia32.cpp:20:
In file included from src/assembler_ia32.h:25:
src/IceOperand.h:105:56: error: member access into incomplete type 'const
Ice::Cfg'
  void emit(const Cfg *Func) const override { emit(Func->getContext()); }
                                                       ^
src/IceDefs.h:39:7: note: forward declaration of 'Ice::Cfg'
class Cfg;
      ^


Any other ideas?

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h#newcod...
src/IceOperand.h:212: // TODO(jvoung): offset should only be int32_t, since we
are ILP32.
On 2014/10/10 17:37:25, stichnot wrote:
> I wonder if this should just be replace now with int32_t (if practical), and
fix
> it later to be more general if ever needed?

Okay, I've added a typedef and used that. I think Karl had a similar typedef,
for global initializers that we may be able to share.

[Jim Stichnoth, 2014-10-13 17:41:06]

lgtm

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h
File src/IceOperand.h (right):

https://codereview.chromium.org/649463002/diff/240001/src/IceOperand.h#newcode21
src/IceOperand.h:21: #include "IceCfg.h"
On 2014/10/10 21:03:39, jvoung wrote:
> On 2014/10/10 17:37:25, stichnot wrote:
> > Why are these includes needed?
> > 
> > In general, I prefer to limit includes in header files to IceDefs.h and
> > IceTypes.h, if possible, and add includes to the .cpp files.
> 
> For this, I added it because the .h file itself does Func->getContext(), and
> Func->allocate(), and therefore depends on the definition of the Cfg.
> 
> 
> Otherwise, futher down the line, if assembler_ia32.h only fwd declares stuff
> from IceOperand.h instead of including it I get:
> 
> In file included from src/assembler_ia32.cpp:20:
> src/assembler_ia32.h:76:30: error: member access into incomplete type 'const
> Ice::ConstantRelocatable'
>       : value_(fixup->value()->getOffset()), fixup_(fixup) {
>                              ^
> I can re-order the includes in assembler_ia32.cpp to have IceOperand.h first,
> then assembler_ia32.h later, but then that has a different issue.
> 
> If I include IceOperand.h in assembler_ia32.h, then I get:
> 
> In file included from src/assembler_ia32.cpp:20:
> In file included from src/assembler_ia32.h:25:
> src/IceOperand.h:64:20: error: member access into incomplete type 'const
> Ice::Cfg'
>     dump(Func, Func->getContext()->getStrDump());
>                    ^
> src/IceDefs.h:39:7: note: forward declaration of 'Ice::Cfg'
> class Cfg;
>       ^
> In file included from src/assembler_ia32.cpp:20:
> In file included from src/assembler_ia32.h:25:
> src/IceOperand.h:105:56: error: member access into incomplete type 'const
> Ice::Cfg'
>   void emit(const Cfg *Func) const override { emit(Func->getContext()); }
>                                                        ^
> src/IceDefs.h:39:7: note: forward declaration of 'Ice::Cfg'
> class Cfg;
>       ^
> 
> 
> Any other ideas?

I'd say for now, just leave it as you had it.  Later we can make a pass to try
to clean up all the include issues.

FWIW, in these cases I usually just move the method definitions into the .cpp
file, and hope that LTO will close the performance gap.  There may need to be
exceptions for methods that we "*know*" are performance-critical.

https://codereview.chromium.org/649463002/diff/720001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/649463002/diff/720001/src/IceDefs.h#newcode75
src/IceDefs.h:75: // NaCl is ILP32, so theoretically we should only need 32-bit
offsets.
NaCl --> PNaCl, right?

The point being that we could be translating to unsandboxed native code on some
architecture that allows 64-bit offsets, but RelocOffsetT applies to the PNaCl
bitcode, not native code.

[jvoung (off chromium), 2014-10-13 22:51:45]

https://codereview.chromium.org/649463002/diff/720001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/649463002/diff/720001/src/IceDefs.h#newcode75
src/IceDefs.h:75: // NaCl is ILP32, so theoretically we should only need 32-bit
offsets.
On 2014/10/13 17:41:06, stichnot wrote:
> NaCl --> PNaCl, right?
> 
> The point being that we could be translating to unsandboxed native code on
some
> architecture that allows 64-bit offsets, but RelocOffsetT applies to the PNaCl
> bitcode, not native code.

Done.

[jvoung (off chromium), 2014-10-13 22:56:38]

Committed patchset #10 (id:1160001) manually as
fe14fb8efa8092d796bda240382b44cff35de4af (presubmit successful).