Subzero. X86. Refactors Address Mode formation.

Subzero. X86. Refactors Address Mode formation.

Refactors the Address Mode optimization interface.

BUG=
R=stichnot@chromium.org

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

[John, 2016-01-20 23:29:57]

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

[John, 2016-01-20 23:29:57]

x86-64-nonsfi pre-reqs.

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

I'd also like to test this CL out before the l-g-t-m, but it doesn't apply
cleanly to latest master and I wasn't able to fix it manually.  Could you upload
an updated patchset?

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:246: Shift = Mem->getShift();
Add an early return here, and pull the rest out of the "else" branch?

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:370: llvm::report_fatal_error("Need to implement
initSandboxPtr() for 64-bit.");
I would add "nonsfi" to this error string.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:381: assert(NeedSandboxing);
This function should probably be checking SandboxingType like the others.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:419: // Addr->Base = SandboxPtr;
Remove the commented code, here and below?

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:238: struct OptAddr {
For better or worse, I *think* we go ahead and call it a class, not struct, when
it has methods or nontrivial ctors.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:945: // SandboxPtr is a Variable that holds the
GlobalOffsetTable address for
This comment should also reflect its use in sandboxing mode.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:947: Variable *SandboxPtr = nullptr;
I have to say, I'm not thrilled with the various uses of "Sandbox" to refer to
both sandboxing and the x86-32 GotVar for nonsfi.

Problem is, I can't think of something that's clearly better.

It would be nice if this and the "Rebased" part of x86 MemOperand could have
names that were more clearly linked.

I'll think about it some more.

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

@~ right now; will address comments (and upload the new patch) tomorrow

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:246: Shift = Mem->getShift();
On 2016/01/22 02:44:10, stichnot wrote:
> Add an early return here, and pull the rest out of the "else" branch?

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:238: struct OptAddr {
On 2016/01/22 02:44:10, stichnot wrote:
> For better or worse, I *think* we go ahead and call it a class, not struct,
when
> it has methods or nontrivial ctors.

But this has no methods, and it has a quasi-dumb, memseting ctor. :(

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:947: Variable *SandboxPtr = nullptr;
On 2016/01/22 02:44:10, stichnot wrote:
> I have to say, I'm not thrilled with the various uses of "Sandbox" to refer to
> both sandboxing and the x86-32 GotVar for nonsfi.
> 
> Problem is, I can't think of something that's clearly better.
> 
> It would be nice if this and the "Rebased" part of x86 MemOperand could have
> names that were more clearly linked.
> 
> I'll think about it some more.

How about RebasingPtr? RebasePtr?

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

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:246: Shift = Mem->getShift();
On 2016/01/22 02:56:21, John wrote:
> On 2016/01/22 02:44:10, stichnot wrote:
> > Add an early return here, and pull the rest out of the "else" branch?
> 
> It's not possible. There's more code after the else that is needed.

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

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:238: struct OptAddr {
On 2016/01/22 02:56:21, John wrote:
> On 2016/01/22 02:44:10, stichnot wrote:
> > For better or worse, I *think* we go ahead and call it a class, not struct,
> when
> > it has methods or nontrivial ctors.
> 
> But this has no methods, and it has a quasi-dumb, memseting ctor. :(

On further thought, I take this back.  If you make it a class, do you also have
to delete default copy ctor and similar boilerplate?  Yuck.

So never mind, I like it better this way. :)

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

git pull'ed
comments addressed.

make presubmit-friendly.

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:246: Shift = Mem->getShift();
On 2016/01/22 03:08:55, stichnot wrote:
> On 2016/01/22 02:56:21, John wrote:
> > On 2016/01/22 02:44:10, stichnot wrote:
> > > Add an early return here, and pull the rest out of the "else" branch?
> > 
> > It's not possible. There's more code after the else that is needed.
> 
> Let me clarify (hopefully I'm not being stupid here...):
> 
>   if (Mem->getIsRebased()) {
>     ...
>     Shift = Mem->getShift();
>     return;
>   }
>   if (Base != nullptr) {
>     ...
>   }

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());
}

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:370: llvm::report_fatal_error("Need to implement
initSandboxPtr() for 64-bit.");
On 2016/01/22 02:44:10, stichnot wrote:
> I would add "nonsfi" to this error string.

Done.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:381: assert(NeedSandboxing);
On 2016/01/22 02:44:10, stichnot wrote:
> This function should probably be checking SandboxingType like the others.

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.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:419: // Addr->Base = SandboxPtr;
On 2016/01/22 02:44:10, stichnot wrote:
> Remove the commented code, here and below?

Done.

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:238: struct OptAddr {
On 2016/01/22 03:08:55, stichnot wrote:
> On 2016/01/22 02:56:21, John wrote:
> > On 2016/01/22 02:44:10, stichnot wrote:
> > > For better or worse, I *think* we go ahead and call it a class, not
struct,
> > when
> > > it has methods or nontrivial ctors.
> > 
> > But this has no methods, and it has a quasi-dumb, memseting ctor. :(
> 
> On further thought, I take this back.  If you make it a class, do you also
have
> to delete default copy ctor and similar boilerplate?  Yuck.
> 
> So never mind, I like it better this way. :)

Not really. In theory, that should be done for structs, too, because the
compiler does generate those ctors/operators for them. It turns out that this
object is just a dumb collection of attributes, and no logic. Therefore, letting
the compiler generate those methods --if needed-- is harmless. And the compiler
will probably do a good job of memcpy() these.

by the way, I am using the copy constructor for this type. :)

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX86Base.h:945: // SandboxPtr is a Variable that holds the
GlobalOffsetTable address for
On 2016/01/22 02:44:10, stichnot wrote:
> This comment should also reflect its use in sandboxing mode.

Done.

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

lgtm

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

https://codereview.chromium.org/1605103002/diff/120001/src/IceTargetLoweringX...
src/IceTargetLoweringX8664.cpp:246: Shift = Mem->getShift();
On 2016/01/22 04:18:07, John wrote:
> On 2016/01/22 03:08:55, stichnot wrote:
> > On 2016/01/22 02:56:21, John wrote:
> > > On 2016/01/22 02:44:10, stichnot wrote:
> > > > Add an early return here, and pull the rest out of the "else" branch?
> > > 
> > > It's not possible. There's more code after the else that is needed.
> > 
> > Let me clarify (hopefully I'm not being stupid here...):
> > 
> >   if (Mem->getIsRebased()) {
> >     ...
> >     Shift = Mem->getShift();
> >     return;
> >   }
> >   if (Base != nullptr) {
> >     ...
> >   }
> 
> 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());
> }

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!

[John, 2016-01-22 15:11:00]

Description was changed from

==========
Subzero. X86. Refactors Address Mode formation.

Refactors the Address Mode optimization interface.

BUG=
==========

to

==========
Subzero. X86. Refactors Address Mode formation.

Refactors the Address Mode optimization interface.

BUG=
R=stichnot@chromium.org

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

[John, 2016-01-22 15:11:01]

Committed patchset #9 (id:160001) manually as
ac2388c385afda17acb3fc242b17b9232ec2262a (presubmit successful).