Subzero: Prototype to simplify makereg calls

This would eliminate a lot of repetitive coding in subzero.
It's troublesome that we have to type (this) in several places
but I don't see a away around it yet and it does not add to the line
of code count at all to have to do this; it's just not aesthetic to me.

If this idea is okay, I can make all the similar changes
in the Mips Subzero port.

BUG=

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

[rkotlerimgtec, 2016-02-04 01:21:38]

Description was changed from

==========
Prototype to simplify makereg calls

This would eliminate a lot of repetitive coding in subzero.
It's troublesome that we have to type (this) in several places
but I don't see a away around it yet and it does not add to the line
of code count at all to have to do this; it's just not aesthetic to me.

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

to

==========
This would eliminate a lot of repetitive coding in subzero.
It's troublesome that we have to type (this) in several places
but I don't see a away around it yet and it does not add to the line
of code count at all to have to do this; it's just not aesthetic to me.

If this idea is okay, I can make all the similar changes
in the Mips Subzero port.

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

[rkotlerimgtec, 2016-02-04 01:21:39]

rkotlerimgtec@gmail.com changed reviewers:
+ jpp@chromium.org, kschimpf@chromium.org, stichnot@chromium.org

[rkotlerimgtec, 2016-02-04 04:27:36]

On 2016/02/04 01:21:53, rkotlerimgtec wrote:

I think if we use c++2011 that we may be able to get rid of the (this) .

I was getting errors about makeReg not being static and those restrictions for
static classes were removed with c++2011

http://en.cppreference.com/w/cpp/language/nested_types

[rkotlerimgtec, 2016-02-04 04:30:36]

..those restrictions for NESTED classes were removed....

On Wed, Feb 3, 2016 at 8:27 PM, <rkotlerimgtec@gmail.com> wrote:

> On 2016/02/04 01:21:53, rkotlerimgtec wrote:
>
> I think if we use c++2011 that we may be able to get rid of the (this) .
>
> I was getting errors about makeReg not being static and those restrictions
for
> static classes were removed with c++2011
> http://en.cppreference.com/w/cpp/language/nested_types
> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2016-02-04 05:45:02]

In general, each target gets a fair amount of leeway in creating its patterns
for lowering, so if you think it's right for MIPS and no one has a better idea,
then go ahead.

One thing to consider is that this pattern can only be used when the type is a
constexpr, i.e. IceType_i32.  Consider the number of occurrences in two fully
implemented targets:

$ git grep makeReg -- src/IceTargetLoweringARM32.cpp | wc -l
108
$ git grep makeReg -- src/IceTargetLoweringARM32.cpp | grep IceType | wc -l
58
$ git grep makeReg -- src/IceTargetLoweringX86BaseImpl.h | wc -l
124
$ git grep makeReg -- src/IceTargetLoweringX86BaseImpl.h | grep IceType | wc -l
48

So you'd likely be affecting about 50 instances, with another ~50 still
necessarily using the original makeReg() because of a non-constexpr type.

One other thing to consider is that the x86 lowering has a special _mov() where
the dest variable is passed by reference and makeReg() is automatically called
if dest==nullptr on entry.  See "typename Traits::Insts::Mov *_mov()".  Overall
it reduces the boilerplate, but I'm not sure that was a great idea in the first
place or that I'd recommend it here.

[rkotlerimgtec, 2016-02-04 06:37:45]

There are some other solutions for situations that don't fit this one by
using variadic templates. I like to keep refining and shrinking the code.
This scheme may evolve over
time but for now it covers a large number of cases.

On Wed, Feb 3, 2016 at 9:45 PM, <stichnot@chromium.org> wrote:

> In general, each target gets a fair amount of leeway in creating its patterns
> for lowering, so if you think it's right for MIPS and no one has a better
idea,
> then go ahead.
>
> One thing to consider is that this pattern can only be used when the type is
a
> constexpr, i.e. IceType_i32.  Consider the number of occurrences in two fully
> implemented targets:
>
> $ git grep makeReg -- src/IceTargetLoweringARM32.cpp | wc -l
> 108
> $ git grep makeReg -- src/IceTargetLoweringARM32.cpp | grep IceType | wc -l
> 58
> $ git grep makeReg -- src/IceTargetLoweringX86BaseImpl.h | wc -l
> 124
> $ git grep makeReg -- src/IceTargetLoweringX86BaseImpl.h | grep IceType | wc
-l
> 48
>
> So you'd likely be affecting about 50 instances, with another ~50 still
> necessarily using the original makeReg() because of a non-constexpr type.
>
> One other thing to consider is that the x86 lowering has a special _mov()
where
> the dest variable is passed by reference and makeReg() is automatically
called
> if dest==nullptr on entry.  See "typename Traits::Insts::Mov *_mov()". 
Overall
> it reduces the boilerplate, but I'm not sure that was a great idea in the
first
> place or that I'd recommend it here.
> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[John, 2016-02-04 16:04:04]

On the need for (this), you could do something like:

class TargetMIPS32 {
...

class Foo {
public:
  Foo(Foo&&) = default;
};

Foo makeRegI32(RegNum) {
  return Foo(this, RegNum);
}
...
};

and let move semantics take this over. However, I would **strongly** discourage
going this route.

I am in general a huge fan of helper classes -- see the Sandboxer for arm, or
the AutoMemSandboxer for x8 -- but I don't think this is really what you want
here.

if the problem is the type parameter, that easily fixable by creating helper
methods:

Variable *makeRegI32(int RegNum == NoReg) {
  return makeReg(IceType_i32, RegNum);
}

which allows you to avoid all the boiler plate with this new helper class.

[Jim Stichnoth, 2016-02-04 16:20:51]

On 2016/02/04 16:04:04, John wrote:
> On the need for (this), you could do something like:
> 
> class TargetMIPS32 {
> ...
> 
> class Foo {
> public:
>   Foo(Foo&&) = default;
> };
> 
> Foo makeRegI32(RegNum) {
>   return Foo(this, RegNum);
> }
> ...
> };
> 
> and let move semantics take this over. However, I would **strongly**
discourage
> going this route.
> 
> I am in general a huge fan of helper classes -- see the Sandboxer for arm, or
> the AutoMemSandboxer for x8 -- but I don't think this is really what you want
> here.
> 
> if the problem is the type parameter, that easily fixable by creating helper
> methods:
> 
> Variable *makeRegI32(int RegNum == NoReg) {
>   return makeReg(IceType_i32, RegNum);
> }
> 
> which allows you to avoid all the boiler plate with this new helper class.

Also for reference, see the getConstantIntXXX() and getConstantZero() helpers,
instead of using just a single getConstantInt() function.

[rkotlerimgtec, 2016-02-04 23:45:21]

The problem is that the following seems to be illegal even in c++11. I
thought it was legal now but I misunderstood the rules.
http://en.cppreference.com/w/cpp/language/nested_types
Now "Declarations in a nested class can use any members of the enclosing
class, following the usual usage rules
<http://en.cppreference.com/w/cpp/language/data_members#Usage> for the
non-static members." but ...
"... but it is otherwise independent and has no special access to the this
<http://en.cppreference.com/w/cpp/language/this> pointer of the enclosing
class"
makeReg needs the this pointer.

In two lines, it's easy to solve this by having:
Variable * x,y,z;
variadic_template_function_make_reg(x,y,z);

But I wanted to do it in one line. Lol.

#include <iostream>
class Outer {
public:
   void foo() {
   }
   class Inner {
      Inner() {
foo();  // cannot call a non static function here because implicityly that
requires
                   // this of the enclosing class.
      }
   };
};

I think I can eliminate most all of the remaining makereg calls too , but
with a slightly different mechanism perhaps.


On Thu, Feb 4, 2016 at 8:20 AM, <stichnot@chromium.org> wrote:

> On 2016/02/04 16:04:04, John wrote:
> > On the need for (this), you could do something like:
> >
> > class TargetMIPS32 {
> > ...
> >
> > class Foo {
> > public:
> >   Foo(Foo&&) = default;
> > };
> >
> > Foo makeRegI32(RegNum) {
> >   return Foo(this, RegNum);
> > }
> > ...
> > };
> >
> > and let move semantics take this over. However, I would **strongly**
> discourage
> > going this route.
> >
> > I am in general a huge fan of helper classes -- see the Sandboxer for arm,
or
> > the AutoMemSandboxer for x8 -- but I don't think this is really what you
want
> > here.
> >
> > if the problem is the type parameter, that easily fixable by creating
helper
> > methods:
> >
> > Variable *makeRegI32(int RegNum == NoReg) {
> >   return makeReg(IceType_i32, RegNum);
> > }
> >
> > which allows you to avoid all the boiler plate with this new helper class.
>
> Also for reference, see the getConstantIntXXX() and getConstantZero()
helpers,
> instead of using just a single getConstantInt() function.
> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2016-02-05 01:52:11]

The idea would be to perhaps follow on with replacing 
Variable *Reg with something like VariableReg Reg with:

Then Variable *Reg = nullptr; would become

VariableReg Reg; // for example

These VariableReg variables could be assigned Variable* 
data, etc.

class VariableReg {
    Variable *V;

  public:
    VariableReg(TargetMIPS32 *Target, Type Ty,
                int32_t RegNum = Variable::NoRegister) {
      V = Target->makeReg(Ty, RegNum);
    }
    VariableReg() { V = nullptr; }
    operator Variable *() { return V; }

    VariableReg &operator=(Variable *VP) {
      V = VP;
      return *this;
    }
  };

[rkotlerimgtec, 2016-02-11 03:54:44]

I'm going to look at solving this problem of "this" using auto. I think
that's the way to do it.
I will post later.

Somethine like

auto x = t->makeReg(), y=t->makeReg()) .....

On Thu, Feb 4, 2016 at 5:52 PM, <rkotlerimgtec@gmail.com> wrote:

> The idea would be to perhaps follow on with replacing
> Variable *Reg with something like VariableReg Reg with:
>
> Then Variable *Reg = nullptr; would become
>
> VariableReg Reg; // for example
>
> These VariableReg variables could be assigned Variable*
> data, etc.
>
> class VariableReg {
>     Variable *V;
>
>   public:
>     VariableReg(TargetMIPS32 *Target, Type Ty,
>                 int32_t RegNum = Variable::NoRegister) {
>       V = Target->makeReg(Ty, RegNum);
>     }
>     VariableReg() { V = nullptr; }
>     operator Variable *() { return V; }
>
>     VariableReg &operator=(Variable *VP) {
>       V = VP;
>       return *this;
>     }
>   };
> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2016-02-11 12:51:04]

Unfortunately, I was not able so far to figure out how to eliminate the
unpleasant looking "(this)" code when I used the helper class.
The problem is that nested classes in C++ have no access to the "this"
pointer of the enclosing class, which makeReg ultimately needs.. It's
possible that there is some clever
way to achieve exactly what I was originally looking for but I don't know
how at this point. This solution with the auto certainly cleans up
the code and makes it more readable IMO. Auto has the nice property of
making the very datatype of the variable be abstract and thus
changeable in the future by just changing the initializer.

On Wed, Feb 10, 2016 at 9:00 PM, <rkotlerimgtec@gmail.com> wrote:

> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[rkotlerimgtec, 2016-02-11 13:00:01]

http://stackoverflow.com/questions/6198224/how-to-refer-to-enclosing-instance...



On Thu, Feb 11, 2016 at 4:51 AM, reed kotler <rkotlerimgtec@gmail.com>
wrote:

> Unfortunately, I was not able so far to figure out how to eliminate the
> unpleasant looking "(this)" code when I used the helper class.
> The problem is that nested classes in C++ have no access to the "this"
> pointer of the enclosing class, which makeReg ultimately needs.. It's
> possible that there is some clever
> way to achieve exactly what I was originally looking for but I don't know
> how at this point. This solution with the auto certainly cleans up
> the code and makes it more readable IMO. Auto has the nice property of
> making the very datatype of the variable be abstract and thus
> changeable in the future by just changing the initializer.
>
> On Wed, Feb 10, 2016 at 9:00 PM, <rkotlerimgtec@gmail.com> wrote:
>
>> https://codereview.chromium.org/1661233002/
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2016-02-12 03:00:19]

I understand the annoyance here of having to use excess boilerplate, more than
should be expected.

One simple possibility is to change makeReg to have the Type parameter default
to IceType_i32:

  Variable *makeReg(Type Ty = IceType_i32, RegNumT RegNum =
RegNumT::NoRegister);

Then you at least simplify to this:

  Variable *T_Carry = makeReg();
  Variable *T_Lo = makeReg();
  Variable *T_Hi = makeReg();
  Variable *T_Hi2 = makeReg();

Another approach (which probably no one will like, including me) is to extend
what I did for the x86 _mov() instruction.  For any instruction with a dest
variable (which is most instructions), pass the dest variable by reference.  If
a nullptr dest is passed in, the instruction calls makeReg() with a suitable
type derived from the other operands, and passes the variable back to the caller
through the ref parameter.

Or, if you don't like magical in/out args, have the instruction return a new
Variable if needed.  Note that most instructions are declared as void functions;
now some may return Variable* instead.

To illustrate with an example.  On a 3-address architecture, the high-level
instruction "A = B + C" is lowered to:
  T1 = B  // _mov
  T2 = C  // _mov
  T3 = T1 + T2  // _add
  A = T3  // _mov

The lowering code and support might look like this:

Variable *_mov(Operand *Src, Variable *Dest = nullptr) {
  if (Dest == nullptr) {
    Dest = makeReg(Src->getType());
  }
  Context.insert<InstMov>(Dest, Src);
  return Dest;
}

Variable *_add(Operand *Src1, Operand *Src0, Variable *Dest = nullptr) {
  if (Dest == nullptr) {
    Dest = makeReg(Src0->getType());
  }
  Context.insert<InstAdd>(Dest, Src0, Src1);
  return Dest;
}

...

Variable *T1 = _mov(B);
Variable *T2 = _mov(C);
Variable *T3 = _add(T2, T1);
_mov(T3, A);

The last _mov isn't quite right because A is not a Variable, but hopefully the
point is made.

The big disadvantage here is that it makes it much harder to follow the ARM
patches.

[rkotlerimgtec, 2016-02-12 03:18:28]

In some places, we will want to allocate floating point registers so making
"int" the default will not help there. We need a separate make for each
register type. I don't see a reason why integer should have any special
significance that would make one expect
it to be a logical default when reading the makeReg calls later.

I like to get the visual footprint of a function as small as possible. It
makes it easier for me to focus on the function as a single thought. Any
information that is not central to me for understanding what is going on, I
like to factor out. Ditto for any information which is
of a lower level of detail than the level of the function.

Back in the day, we had a rule that a function had to always fit on a page
but smaller is always better and as time passes, functions can grow in size
and complexity easily.

The other point is that I think we wanted the code to look like assembly so
the last case deviates from that but as long as style is consistent
throughout the compiler, we can do as we wish.

We probably don't need the Variable *, i.e. auto is enough.

If we are willing to live with the T1, T2, T3 idea instead of more
descriptive names,
then we could just as well allocated an array and initialize it all at once.
tmp[3]. I kind of like the descriptive names src, dest, etc.

I prefer to have all the temporaries declared on one line.
If we use auto, then to me these are just declarations (not statements) and
then
they can all appear on the same line.

"Auto" frees us from needing to put an actual type name here and thus the
types it can be changed easily later by just changing the value type that
the function returns.

On Thu, Feb 11, 2016 at 7:00 PM, <stichnot@chromium.org> wrote:

> I understand the annoyance here of having to use excess boilerplate, more
than
> should be expected.
>
> One simple possibility is to change makeReg to have the Type parameter
default
> to IceType_i32:
>
>   Variable *makeReg(Type Ty = IceType_i32, RegNumT RegNum =
> RegNumT::NoRegister);
>
> Then you at least simplify to this:
>
>   Variable *T_Carry = makeReg();
>   Variable *T_Lo = makeReg();
>   Variable *T_Hi = makeReg();
>   Variable *T_Hi2 = makeReg();
>
> Another approach (which probably no one will like, including me) is to extend
> what I did for the x86 _mov() instruction.  For any instruction with a dest
> variable (which is most instructions), pass the dest variable by reference. 
If
> a nullptr dest is passed in, the instruction calls makeReg() with a suitable
> type derived from the other operands, and passes the variable back to the
caller
> through the ref parameter.
>
> Or, if you don't like magical in/out args, have the instruction return a new
> Variable if needed.  Note that most instructions are declared as void
functions;
> now some may return Variable* instead.
>
> To illustrate with an example.  On a 3-address architecture, the high-level
> instruction "A = B + C" is lowered to:
>   T1 = B  // _mov
>   T2 = C  // _mov
>   T3 = T1 + T2  // _add
>   A = T3  // _mov
>
> The lowering code and support might look like this:
>
> Variable *_mov(Operand *Src, Variable *Dest = nullptr) {
>   if (Dest == nullptr) {
>     Dest = makeReg(Src->getType());
>   }
>   Context.insert<InstMov>(Dest, Src);
>   return Dest;
> }
>
> Variable *_add(Operand *Src1, Operand *Src0, Variable *Dest = nullptr) {
>   if (Dest == nullptr) {
>     Dest = makeReg(Src0->getType());
>   }
>   Context.insert<InstAdd>(Dest, Src0, Src1);
>   return Dest;
> }
>
> ...
>
> Variable *T1 = _mov(B);
> Variable *T2 = _mov(C);
> Variable *T3 = _add(T2, T1);
> _mov(T3, A);
>
> The last _mov isn't quite right because A is not a Variable, but hopefully
the
> point is made.
>
> The big disadvantage here is that it makes it much harder to follow the ARM
> patches.
> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2016-02-12 03:31:26]

On 2016/02/12 03:18:28, rkotlerimgtec wrote:
> In some places, we will want to allocate floating point registers so making
> "int" the default will not help there. We need a separate make for each
> register type. I don't see a reason why integer should have any special
> significance that would make one expect
> it to be a logical default when reading the makeReg calls later.

If you "git grep -w makeReg", you will see that the large majority of type
arguments are i32.  Without careful study, I think that's just a reflection of a
32-bit architecture.  That's why I suggested it as the default.  If you need a
different type, or a specific register, just specify it.  Or you can go wild and
make a version of makeReg() for every type, kind of like the getConstantInt*()
methods.

> We probably don't need the Variable *, i.e. auto is enough.

Sure, though I would say "auto *" is better than "auto".

> If we are willing to live with the T1, T2, T3 idea instead of more
> descriptive names,
> then we could just as well allocated an array and initialize it all at once.
> tmp[3]. I kind of like the descriptive names src, dest, etc.

T1/T2/T3 is just how I always scribble these examples on a whiteboard.  No
deeper meaning was implied. :)

[rkotlerimgtec, 2016-02-15 07:44:07]

Patchset #4 (id:60001) has been deleted

[rkotlerimgtec, 2016-02-15 07:54:30]

On 2016/02/12 03:31:26, stichnot wrote:
> On 2016/02/12 03:18:28, rkotlerimgtec wrote:
> > In some places, we will want to allocate floating point registers so making
> > "int" the default will not help there. We need a separate make for each
> > register type. I don't see a reason why integer should have any special
> > significance that would make one expect
> > it to be a logical default when reading the makeReg calls later.
> 
> If you "git grep -w makeReg", you will see that the large majority of type
> arguments are i32.  Without careful study, I think that's just a reflection of
a
> 32-bit architecture.  That's why I suggested it as the default.  If you need a
> different type, or a specific register, just specify it.  Or you can go wild
and
> make a version of makeReg() for every type, kind of like the getConstantInt*()
> methods.
> 
> > We probably don't need the Variable *, i.e. auto is enough.
> 
> Sure, though I would say "auto *" is better than "auto".
> 
> > If we are willing to live with the T1, T2, T3 idea instead of more
> > descriptive names,
> > then we could just as well allocated an array and initialize it all at once.
> > tmp[3]. I kind of like the descriptive names src, dest, etc.
> 
> T1/T2/T3 is just how I always scribble these examples on a whiteboard.  No
> deeper meaning was implied. :)

I have changed things to auto*. I'm okay with that for now since we are using
Variable* everywhere.

I prefer what we are doing now with RegNumT though where we have a method that
tells us whether the data has a value instead of using pointers being
null or not. Pointers being null does not really tell you anything; it's a
concrete way to implement "not a value" but it has no universal abstract
meaning.

It would be nice in the future to get rid of all Variable* and have a VariableT
class with a "hasValue() and hasNoValue()" member function.
We can still implement that class with a pointer.

Then we could use auto everywhere instead of this auto*.

Looking a bit ahead, when we go to C++14 we can use decltype which is
better than auto and allows & to work more transparently in such case.

[rkotlerimgtec, 2016-02-15 19:59:44]

I'm willing to make these changes for all the ports, not just Mips if you
want.


On Sun, Feb 14, 2016 at 11:54 PM, <rkotlerimgtec@gmail.com> wrote:

> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2016-02-17 03:29:45]

This looks GTM, but I'm pretty sure you didn't "make presubmit" (which would
have done a make format).

https://codereview.chromium.org/1661233002/diff/80001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1661233002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:815: ReturnReg = makeReg(IceType_i32,
RegMIPS32::Reg_V0);
Optional: I32Reg could take an optional argument RegNumT argument, like makeReg,
and could then be used here.

[rkotlerimgtec, 2016-02-17 04:27:30]

https://codereview.chromium.org/1661233002/diff/80001/src/IceTargetLoweringMI...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/1661233002/diff/80001/src/IceTargetLoweringMI...
src/IceTargetLoweringMIPS32.cpp:815: ReturnReg = makeReg(IceType_i32,
RegMIPS32::Reg_V0);
On 2016/02/17 03:29:45, stichnot wrote:
> Optional: I32Reg could take an optional argument RegNumT argument, like
makeReg,
> and could then be used here.

Done.

[Jim Stichnoth, 2016-02-17 13:37:05]

Description was changed from

==========
This would eliminate a lot of repetitive coding in subzero.
It's troublesome that we have to type (this) in several places
but I don't see a away around it yet and it does not add to the line
of code count at all to have to do this; it's just not aesthetic to me.

If this idea is okay, I can make all the similar changes
in the Mips Subzero port.

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

to

==========
This would eliminate a lot of repetitive coding in subzero.
It's troublesome that we have to type (this) in several places
but I don't see a away around it yet and it does not add to the line
of code count at all to have to do this; it's just not aesthetic to me.

If this idea is okay, I can make all the similar changes
in the Mips Subzero port.

BUG=

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

[Jim Stichnoth, 2016-02-17 13:37:06]

Committed patchset #5 (id:100001) manually as
953568f83d6933c8175754e3665ec844e14d5b51 (presubmit successful).

[Jim Stichnoth, 2016-02-17 13:41:47]

Whoops, I forgot to edit the Description so that "git log --oneline" looks
sensible.

Reed, in the future can you please do that prior to the initial "git cl upload".

Words like "prototype" and future plans for the CL should go in the
"Publish+Mail Comments" message or in a CL comment attached to a source file,
rather than the CL Description field.

[rkotlerimgtec, 2016-02-17 14:28:11]

okay. gotcha.

On Wed, Feb 17, 2016 at 5:41 AM, <stichnot@chromium.org> wrote:

> Whoops, I forgot to edit the Description so that "git log --oneline" looks
> sensible.
>
> Reed, in the future can you please do that prior to the initial "git cl
upload".
>
> Words like "prototype" and future plans for the CL should go in the
> "Publish+Mail Comments" message or in a CL comment attached to a source file,
> rather than the CL Description field.
> https://codereview.chromium.org/1661233002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at https://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.