Subzero. X8664. Enables RIP-based addressing mode.

Subzero. X8664. Enables RIP-based addressing mode.


BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
R=stichnot@chromium.org

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

[John, 2016-01-22 23:38:29]

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

[Jim Stichnoth, 2016-01-23 01:57:47]

lgtm

https://codereview.chromium.org/1616103002/diff/110001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1616103002/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:302: static constexpr RelocOffsetT
OffsetFromNextInstruction = 1;
What do you think about using typeWidthInBytes(Ty) in this method?

https://codereview.chromium.org/1616103002/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:3398: auto *const Fixup = imm.fixup();
Fortunately I didn't have to scan too far from here to figure out what Fixup's
type is...

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:307: if (!Var->hasReg()) {
Optional: You could skip this condition if you want, since getRegNum() returns
NoRegister if !hasReg().

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:311: int32_t RegNum = Var->getRegNum();
const

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:349: // We don't return early because we still
need to zero extend Index.
Is the "don't return early" comment because of my derp comment in the previous
CL?  If so, don't consider it necessary here. :)

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:371: // Otherwise, the lowering generated an
memory operand with two registers.
s/an/a/

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:418: llvm::report_fatal_error("Mem pointer should
be a 32-bit integer.");
Here and below, would it make more sense to say "GPR" instead of "integer"?

[John, 2016-01-26 19:18:46]

Description was changed from

==========
Subzero. X8664. Enables RIP-based addressing mode.

... and nonsfi.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

to

==========
Subzero. X8664. Enables RIP-based addressing mode.


BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

[John, 2016-01-26 19:44:04]

Description was changed from

==========
Subzero. X8664. Enables RIP-based addressing mode.


BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
==========

to

==========
Subzero. X8664. Enables RIP-based addressing mode.


BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4077
R=stichnot@chromium.org

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

[John, 2016-01-26 19:44:08]

Committed patchset #14 (id:250001) manually as
d1bd1d330c8eb8116811ad4dc01fe636b80b86cb (presubmit successful).

[John, 2016-01-26 19:44:25]

https://codereview.chromium.org/1616103002/diff/110001/src/IceAssemblerX86Bas...
File src/IceAssemblerX86BaseImpl.h (right):

https://codereview.chromium.org/1616103002/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:302: static constexpr RelocOffsetT
OffsetFromNextInstruction = 1;
On 2016/01/23 01:57:46, stichnot wrote:
> What do you think about using typeWidthInBytes(Ty) in this method?

Discussed offline. keeping as is.

https://codereview.chromium.org/1616103002/diff/110001/src/IceAssemblerX86Bas...
src/IceAssemblerX86BaseImpl.h:3398: auto *const Fixup = imm.fixup();
On 2016/01/23 01:57:46, stichnot wrote:
> Fortunately I didn't have to scan too far from here to figure out what Fixup's
> type is...

My arguments for auto here:

1) the following line

AssemblerFixup *const Fixup = imm.fixup();

contains way too many "fixups" in it. (dummy reason, I know, but it is still
valid. :P)

2)  Fixup is not really used here for anything, except to cache imm.fixup(). The
previous code used imm.fixup() as a function argument, ergo the return type was
not known to being with. auto here is not making anything worse than the call

emitFixup(imm.fixup()); // what's typedecl(imm.fixup())?

3) auto is great for "throw away" temporaries like this. I personally refrain
from using temporaries to cache function return values simply because I don't
want to spell out types.

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8664.cpp (right):

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:307: if (!Var->hasReg()) {
On 2016/01/23 01:57:47, stichnot wrote:
> Optional: You could skip this condition if you want, since getRegNum() returns
> NoRegister if !hasReg().

I did not know that. I will leave this as is -- explicit check will not leave
the reader wondering "what if !Var->hasReg()"

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:311: int32_t RegNum = Var->getRegNum();
On 2016/01/23 01:57:47, stichnot wrote:
> const

Done.

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:349: // We don't return early because we still
need to zero extend Index.
On 2016/01/23 01:57:47, stichnot wrote:
> Is the "don't return early" comment because of my derp comment in the previous
> CL?  If so, don't consider it necessary here. :)

no, it is not. The previous comment said "but we still need to truncate
Mem.Index (if any) to 32-bit." That obscure comment really meant "we can't
return early because with need to movzx(Index)."

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:371: // Otherwise, the lowering generated an
memory operand with two registers.
On 2016/01/23 01:57:47, stichnot wrote:
> s/an/a/

Done.

https://codereview.chromium.org/1616103002/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:418: llvm::report_fatal_error("Mem pointer should
be a 32-bit integer.");
On 2016/01/23 01:57:47, stichnot wrote:
> Here and below, would it make more sense to say "GPR" instead of "integer"?

I tend to think about GPRs/Regs in the assembler. In the lowering, I believe it
makes more sense to think about temporaries, integer types (which will
eventually be mapped to 32-bit registers, maybe) and other higher-level
entities. but no strong opinions, so done.