Subzero. X86. Refactors Address Mode formation.

src/IceTargetLoweringX8664.cpp

 //===- subzero/src/IceTargetLoweringX8664.cpp - x86-64 lowering -----------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 201 matching lines @@
   // to avoid leaking the upper 32-bits (i.e., the sandbox address.)
   AutoBundle _(this);
   _push(_0);
   Context.insert<typename Traits::Insts::Store>(ebp, TopOfStack);
 }
 
 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;
+    // 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 && 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;
-    Shift = Mem->getShift();
+  } 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
   // GPR prior to being used. A LEA is needed if Mem.Offset is a constant
   // relocatable, or if Mem.Offset is negative. In both these cases, the LEA is
   // needed to ensure the sandboxed memory operand will only use the lower
   // 32-bits of T+Offset.
   bool NeedsLea = false;
   if (const auto *Offset = Mem->getOffset()) {
     if (llvm::isa<ConstantRelocatable>(Offset)) {
@@ skipping 54 matching lines @@
                    Traits::X86OperandMem::DefaultSegment, NotRebased));
 
     T = makeReg(IceType_i64, RegNum);
     _movzx(T, NewT);
     Shift = 0;
     Offset = nullptr;
   }
 
   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);
 }
 
 void TargetX8664::_sub_sp(Operand *Adjustment) {
   Variable *rsp =
       getPhysicalRegister(Traits::RegisterSet::Reg_rsp, IceType_i64);
   if (!NeedSandboxing) {
     _sub(rsp, Adjustment);
     return;
   }
 
   Variable *esp =
       getPhysicalRegister(Traits::RegisterSet::Reg_esp, IceType_i32);
   Variable *r15 =
       getPhysicalRegister(Traits::RegisterSet::Reg_r15, IceType_i64);
 
   // .bundle_start
   // sub Adjustment, %esp
   // add %r15, %rsp
   // .bundle_end
   AutoBundle _(this);
   _redefined(Context.insert<InstFakeDef>(esp, rsp));
   _sub(esp, Adjustment);
   _redefined(Context.insert<InstFakeDef>(rsp, esp));
   _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());
   Context.setInsertPoint(Context.getCur());
   Variable *r15 =
       getPhysicalRegister(Traits::RegisterSet::Reg_r15, IceType_i64);
   Context.insert<InstFakeDef>(r15);
   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;
 
   if (!NeedSandboxing) {
     Variable *T = makeReg(IceType_i64);
     _movzx(T, JumpTarget);
     JumpTarget = T;
   } else {
     Variable *T = makeReg(IceType_i32);
     Variable *T64 = makeReg(IceType_i64);
@@ skipping 775 matching lines @@
 #define X(tag, sizeLog2, align, elts, elty, str)                               \
   static_assert(_table1_##tag == _table2_##tag,                                \
                 "Inconsistency between ICETYPEX8664_TABLE and ICETYPE_TABLE");
 ICETYPE_TABLE
 #undef X
 } // end of namespace dummy3
 } // end of anonymous namespace
 
 } // end of namespace X8664
 } // end of namespace Ice