Subzero. X86. Refactors Address Mode formation.

src/IceTargetLoweringX8664.cpp

Index: src/IceTargetLoweringX8664.cpp
diff --git a/src/IceTargetLoweringX8664.cpp b/src/IceTargetLoweringX8664.cpp
index 6fe92bf2d8d93c43cb7dd1f5755aea67d578f425..95e15ddd0415f80130cb8ee0c5c31ceb7fde0b6c 100644
--- a/src/IceTargetLoweringX8664.cpp
+++ b/src/IceTargetLoweringX8664.cpp
@@ -219,33 +219,48 @@ 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.
-  assert(NeedSandboxing);
+  if (SandboxingType == ST_None) {
+    return Mem;
+  }
+
+  if (SandboxingType == ST_Nonsfi) {
+    llvm::report_fatal_error(
+        "_sandbox_mem_reference not implemented for nonsfi");
+  }
+
   Variable *Base = Mem->getBase();
   Variable *Index = Mem->getIndex();
   uint16_t Shift = 0;
-  Variable *r15 =
+  Variable *ZeroReg =
       getPhysicalRegister(Traits::RegisterSet::Reg_r15, IceType_i64);
   Constant *Offset = Mem->getOffset();
   Variable *T = nullptr;
 
   if (Mem->getIsRebased()) {
     // If Mem.IsRebased, then we don't need to update Mem to contain a reference
-    // to %r15, but we still need to truncate Mem.Index (if any) to 32-bit.
-    assert(r15 == Base);
-    T = Index;
-    Shift = Mem->getShift();
-  } else if (Base != nullptr && Index != nullptr) {
-    // Another approach could be to emit an
-    //
-    //   lea Mem, %T
-    //
-    // And then update Mem.Base = r15, Mem.Index = T, Mem.Shift = 0
-    llvm::report_fatal_error("memory reference contains base and index.");
-  } else if (Base != nullptr) {
-    T = Base;
-  } else if (Index != nullptr) {
-    T = Index;
+    // 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();

[Jim Stichnoth, 2016/01/22 02:44:10]

Add an early return here, and pull the rest out of the "else" branch?

[John, 2016/01/22 02:56:21]

It's not possible. There's more code after the else that is needed.

[Jim Stichnoth, 2016/01/22 03:08:55]

Let me clarify (hopefully I'm not being stupid here...):

  if (Mem->getIsRebased()) {
    ...
    Shift = Mem->getShift();
    return;
  }
  if (Base != nullptr) {
    ...
  }

[John, 2016/01/22 04:18:07]

That would not work:

void TargetX8664::_sandbox_mem_reference(...) {
  if (Mem.IsRebased) {
    T, Shift <- Mem.ScaledIndex;
  } else {
    if (Mem.Base)
      T, Shift <- Mem.Base, 0
    if (Mem.ScaledIndex) {
      assert(T == nullptr); // i.e., Base == nullptr
      T, Shift <- Mem.ScaledIndex;
  }

  // The new address operand needs to be created, the if can't return early.

  T = _zero_extend(T) to 64 bits;
  return MemoryOperand::create(ZeroReg, T, Shift, Mem.getOffset());
}

[Jim Stichnoth, 2016/01/22 04:29:40]

OK, it turns out I was indeed stupid.  I misread the patch and thought the
NeedsLea comment below was the start of a new method.  Sorry!

+  } else {
+    if (Base != nullptr) {
+      if (Base->isRematerializable()) {
+        ZeroReg = Base;
+      } else {
+        T = Base;
+      }
+    }
+
+    if (Index != nullptr) {
+      assert(!Index->isRematerializable());
+      if (T != nullptr) {
+        llvm::report_fatal_error("memory reference contains base and index.");
+      }
+      T = Index;
+      Shift = Mem->getShift();
+    }
   }
 
   // NeedsLea is a flags indicating whether Mem needs to be materialized to a
@@ -320,7 +335,7 @@ Traits::X86OperandMem *TargetX8664::_sandbox_mem_reference(X86OperandMem *Mem) {
 
   static constexpr bool IsRebased = true;
   return Traits::X86OperandMem::create(
-      Func, Mem->getType(), r15, Offset, T, Shift,
+      Func, Mem->getType(), ZeroReg, Offset, T, Shift,
       Traits::X86OperandMem::DefaultSegment, IsRebased);
 }
 
@@ -348,6 +363,20 @@ void TargetX8664::_sub_sp(Operand *Adjustment) {
   _add(rsp, r15);
 }
 
+void TargetX8664::initSandboxPtr() {
+  switch (SandboxingType) {
+  case ST_Nonsfi:
+    // Probably no implementation is needed, but error to be safe for now.
+    llvm::report_fatal_error("Need to implement initSandboxPtr() for 64-bit.");

[Jim Stichnoth, 2016/01/22 02:44:10]

I would add "nonsfi" to this error string.

[John, 2016/01/22 04:18:07]

Done.

+  case ST_NaCl:
+    SandboxPtr = getPhysicalRegister(Traits::RegisterSet::Reg_r15, IceType_i64);
+    break;
+  case ST_None:
+    // nothing.
+    break;
+  }
+}
+
 void TargetX8664::initSandbox() {
   assert(NeedSandboxing);

[Jim Stichnoth, 2016/01/22 02:44:10]

This function should probably be checking SandboxingType like the others.

[John, 2016/01/22 04:18:07]

It is: NeedSandboxing --> SandboxingType == ST_NaCl;

There are just too many occurrences of NeedSandboxing throughout the code base,
so I'll leave removing it for another CL. 

   Context.init(Func->getEntryNode());
@@ -358,6 +387,48 @@ void TargetX8664::initSandbox() {
   Context.insert<InstFakeUse>(r15);
 }
 
+namespace {
+bool isRematerializable(const Variable *Var) {
+  return Var != nullptr && Var->isRematerializable();
+}
+} // end of anonymous namespace
+
+bool TargetX8664::legalizeOptAddrForSandbox(OptAddr *Addr) {
+  if (SandboxingType == ST_Nonsfi) {
+    llvm::report_fatal_error("Nonsfi not yet implemented for x8664.");
+  }
+
+  if (isRematerializable(Addr->Base)) {
+    if (Addr->Index == SandboxPtr) {
+      Addr->Index = nullptr;
+      Addr->Shift = 0;
+    }
+    return true;
+  }
+
+  if (isRematerializable(Addr->Index)) {
+    if (Addr->Base == SandboxPtr) {
+      Addr->Base = nullptr;
+    }
+    return true;
+  }
+
+  assert(Addr->Base != SandboxPtr && Addr->Index != SandboxPtr);
+
+  if (Addr->Base == nullptr) {
+    // Addr->Base = SandboxPtr;

[Jim Stichnoth, 2016/01/22 02:44:10]

Remove the commented code, here and below?

[John, 2016/01/22 04:18:07]

Done.

+    return true;
+  }
+
+  if (Addr->Index == nullptr) {
+    // Addr->Index = SandboxPtr;
+    // Addr->Shift = 0;
+    return true;
+  }
+
+  return false;
+}
+
 void TargetX8664::lowerIndirectJump(Variable *JumpTarget) {
   std::unique_ptr<AutoBundle> Bundler;