Subzero. X8664. Enables RIP-based addressing mode.

src/IceTargetLoweringX8664.cpp

Index: src/IceTargetLoweringX8664.cpp
diff --git a/src/IceTargetLoweringX8664.cpp b/src/IceTargetLoweringX8664.cpp
index e46569bea008add9457f86771fb2048d54247a9a..73c2cb8a85f1f9e2c4c5c28d6fb01b5206c599ae 100644
--- a/src/IceTargetLoweringX8664.cpp
+++ b/src/IceTargetLoweringX8664.cpp
@@ -294,10 +294,31 @@ void TargetX8664::emitGetIP(CfgNode *Node) {
   (void)Node;
 }
 
+namespace {
+bool isAssignedToRspOrRbp(const Variable *Var) {
+  if (Var == nullptr) {
+    return false;
+  }
+
+  if (Var->isRematerializable()) {
+    return true;
+  }
+
+  if (!Var->hasReg()) {

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

Optional: You could skip this condition if you want, since getRegNum() returns
NoRegister if !hasReg().

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

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

+    return false;
+  }
+
+  int32_t RegNum = Var->getRegNum();

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

const

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

Done.

+  if ((RegNum == Traits::RegisterSet::Reg_rsp) ||
+      (RegNum == Traits::RegisterSet::Reg_rbp)) {
+    return true;
+  }
+
+  return false;
+}
+} // end of anonymous namespace
+
 Traits::X86OperandMem *TargetX8664::_sandbox_mem_reference(X86OperandMem *Mem) {
-  // In x86_64-nacl, all memory references are relative to %r15 (i.e., %rzp.)
-  // NaCl sandboxing also requires that any registers that are not %rsp and
-  // %rbp to be 'truncated' to 32-bit before memory access.
   if (SandboxingType == ST_None) {
     return Mem;
   }
@@ -307,25 +328,37 @@ Traits::X86OperandMem *TargetX8664::_sandbox_mem_reference(X86OperandMem *Mem) {
         "_sandbox_mem_reference not implemented for nonsfi");
   }
 
+  // In x86_64-nacl, all memory references are relative to a base register
+  // (%r15, %rsp, %rbp, or %rip).
+
   Variable *Base = Mem->getBase();
   Variable *Index = Mem->getIndex();
   uint16_t Shift = 0;
-  Variable *ZeroReg =
-      getPhysicalRegister(Traits::RegisterSet::Reg_r15, IceType_i64);
+  Variable *ZeroReg = RebasePtr;
   Constant *Offset = Mem->getOffset();
   Variable *T = nullptr;
 
+  if (Base == nullptr && Index == nullptr) {
+    // Mem is RIP-relative. There's no need to rebase it.
+    return Mem;
+  }
+
   if (Mem->getIsRebased()) {
-    // If Mem.IsRebased, then we don't need to update Mem to contain a reference
-    // to a valid base register (%r15, %rsp, or %rbp), but we still need to
-    // truncate Mem.Index (if any) to 32-bit.
-    assert(ZeroReg == Base || Base->isRematerializable());
-    T = makeReg(IceType_i32);
-    _mov(T, Index);
-    Shift = Mem->getShift();
+    // If Mem.IsRebased, then we don't need to update Mem, as it's already been
+    // updated to contain a reference to one of %rsp, %rbp, or %r15.
+    // We don't return early because we still need to zero extend Index.

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

Is the "don't return early" comment because of my derp comment in the previous
CL?  If so, don't consider it necessary here. :)

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

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)."

+    assert(ZeroReg == Base || isAssignedToRspOrRbp(Base));
+    ZeroReg = Base;
+    if (Index != nullptr) {
+      T = makeReg(IceType_i32);
+      _mov(T, Index);
+      Shift = Mem->getShift();
+    }
   } else {
     if (Base != nullptr) {
-      if (Base->isRematerializable()) {
+      // If Base is a valid base pointer we don't need to use the RebasePtr. By
+      // doing this we might save us the need to zero extend the memory operand.
+      if (isAssignedToRspOrRbp(Base)) {
         ZeroReg = Base;
       } else {
         T = Base;
@@ -334,11 +367,23 @@ Traits::X86OperandMem *TargetX8664::_sandbox_mem_reference(X86OperandMem *Mem) {
 
     if (Index != nullptr) {
       assert(!Index->isRematerializable());
+      // If Index is not nullptr, it is mandatory that T is a nullptr.
+      // Otherwise, the lowering generated an memory operand with two registers.

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

s/an/a/

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

Done.

+      // Note that Base might still be non-nullptr, but it must be a valid
+      // base register.
       if (T != nullptr) {
         llvm::report_fatal_error("memory reference contains base and index.");
       }
-      T = Index;
-      Shift = Mem->getShift();
+      // If the Index is not shifted, and it is a Valid Base, and the ZeroReg is
+      // still RebasePtr, then we do ZeroReg = Index, and hopefully prevent the
+      // need to zero-extend the memory operand (which may still happen -- see
+      // NeedLea below.)
+      if (Shift == 0 && isAssignedToRspOrRbp(Index) && ZeroReg == RebasePtr) {
+        ZeroReg = Index;
+      } else {
+        T = Index;
+        Shift = Mem->getShift();
+      }
     }
   }
 
@@ -362,21 +407,20 @@ Traits::X86OperandMem *TargetX8664::_sandbox_mem_reference(X86OperandMem *Mem) {
     if (T->hasReg()) {
       RegNum = Traits::getGprForType(IceType_i64, T->getRegNum());
       RegNum32 = Traits::getGprForType(IceType_i32, RegNum);
-      switch (RegNum) {
-      case Traits::RegisterSet::Reg_rsp:
-      case Traits::RegisterSet::Reg_rbp:
-        // Memory operands referencing rsp/rbp do not need to be sandboxed.
-        return Mem;
-      }
+      // At this point, if T was assigned to rsp/rbp, then we would have already
+      // made this the ZeroReg.
+      assert(RegNum != Traits::RegisterSet::Reg_rsp);
+      assert(RegNum != Traits::RegisterSet::Reg_rbp);
     }
 
     switch (T->getType()) {
     default:
+      llvm::report_fatal_error("Mem pointer should be a 32-bit integer.");

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

Here and below, would it make more sense to say "GPR" instead of "integer"?

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

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.

     case IceType_i64:
       // Even though "default:" would also catch T.Type == IceType_i64, an
       // explicit 'case IceType_i64' shows that memory operands are always
       // supposed to be 32-bits.
-      llvm::report_fatal_error("Mem pointer should be 32-bit.");
+      llvm::report_fatal_error("Mem pointer should not be a 64-bit integer.");
     case IceType_i32: {
       Variable *T64 = makeReg(IceType_i64, RegNum);
       auto *Movzx = _movzx(T64, T);