Subzero: Basic unconditional branch for Mips.

Subzero: Basic unconditional branch for Mips.

This is part of ARM patch:
https://codereview.chromium.org/1151663004/

BUG=
R=stichnot@chromium.org

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

[rkotlerimgtec, 2016-02-23 04:03:47]

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

[Jim Stichnoth, 2016-02-23 06:18:51]

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:115: InstMIPS32Label::InstMIPS32Label(Cfg *Func,
TargetMIPS32 *Target)
You may want to consider alphabetizing or otherwise organizing the methods here.
 Your call.

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:120: return ".L" + Func->getFunctionName() + "$local$__" +
std::to_string(Number);
This method should probably start with something like:

  if (!BuildDefs::dump())
    return IceString();

And while you're at it, maybe do the same for InstX86Label::getName() and
InstARM32Label::getName() ?

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:255: }
Leave something here, like a TODO or an assert?

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.h#newcode314
src/IceInstMIPS32.h:314: void emitIAS(const Cfg *Func) const override;
Missing definitions of emitIAS() and dump().

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.h#newcode315
src/IceInstMIPS32.h:315: void dump(const Cfg *Func) const override;
There should probably be a static classof() method as well.

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
File tests_lit/llvm2ice_tests/uncond_br.ll (right):

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
tests_lit/llvm2ice_tests/uncond_br.ll:16: ; <label>:2                           
           ; preds = %2, %0
I know this was basically auto-generated, but it would be more readable and
probably more reliable if you give this label an actual name, like:

...
  br label %target
target:
...

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
tests_lit/llvm2ice_tests/uncond_br.ll:26: ; MIPS32:   b  8 <.Luncond1$__1>
Does the "8" refer to a byte offset within the function?  If so, you probably
want to future-proof this because offsets are likely to change once the prolog
is implemented.  E.g.:

; MIPS32: b {{.*}} <.Luncond1$__1>

BTW, I made the comment about providing a trivial definition of Label::getName()
for a MINIMAL build, but I guess that would be a problem here because this lit
test is run in MINIMAL builds...

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
tests_lit/llvm2ice_tests/uncond_br.ll:31: 
Just remove the rest of this.

[rkotlerimgtec, 2016-02-23 23:17:20]

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:115: InstMIPS32Label::InstMIPS32Label(Cfg *Func,
TargetMIPS32 *Target)
On 2016/02/23 06:18:51, stichnot wrote:
> You may want to consider alphabetizing or otherwise organizing the methods
here.
>  Your call.

Let's alphabetize in another patch. Otherwise it will make it hard to see what
is going on in this patch. I will do it right away after this patch.

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:120: return ".L" + Func->getFunctionName() + "$local$__" +
std::to_string(Number);
On 2016/02/23 06:18:51, stichnot wrote:
> This method should probably start with something like:
> 
>   if (!BuildDefs::dump())
>     return IceString();
> 
> And while you're at it, maybe do the same for InstX86Label::getName() and
> InstARM32Label::getName() ?

Done.

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:255: }
On 2016/02/23 06:18:51, stichnot wrote:
> Leave something here, like a TODO or an assert?

Done.

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.h#newcode314
src/IceInstMIPS32.h:314: void emitIAS(const Cfg *Func) const override;
On 2016/02/23 06:18:51, stichnot wrote:
> Missing definitions of emitIAS() and dump().

Done.

https://codereview.chromium.org/1724643002/diff/1/src/IceInstMIPS32.h#newcode315
src/IceInstMIPS32.h:315: void dump(const Cfg *Func) const override;
On 2016/02/23 06:18:51, stichnot wrote:
> There should probably be a static classof() method as well.

Done.

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
File tests_lit/llvm2ice_tests/uncond_br.ll (right):

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
tests_lit/llvm2ice_tests/uncond_br.ll:16: ; <label>:2                           
           ; preds = %2, %0
On 2016/02/23 06:18:51, stichnot wrote:
> I know this was basically auto-generated, but it would be more readable and
> probably more reliable if you give this label an actual name, like:
> 
> ...
>   br label %target
> target:
> ...

Done.

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
tests_lit/llvm2ice_tests/uncond_br.ll:26: ; MIPS32:   b  8 <.Luncond1$__1>
On 2016/02/23 06:18:51, stichnot wrote:
> Does the "8" refer to a byte offset within the function?  If so, you probably
> want to future-proof this because offsets are likely to change once the prolog
> is implemented.  E.g.:
> 
> ; MIPS32: b {{.*}} <.Luncond1$__1>
> 
> BTW, I made the comment about providing a trivial definition of
Label::getName()
> for a MINIMAL build, but I guess that would be a problem here because this lit
> test is run in MINIMAL builds...

i'm not exactly sure what the 8 means. it's something from the objdump program.
i'm just emitting a branch in assembly to a label.

https://codereview.chromium.org/1724643002/diff/1/tests_lit/llvm2ice_tests/un...
tests_lit/llvm2ice_tests/uncond_br.ll:31: 
On 2016/02/23 06:18:51, stichnot wrote:
> Just remove the rest of this.

Done.

[Jim Stichnoth, 2016-02-24 00:11:55]

https://codereview.chromium.org/1724643002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/uncond_br.ll (right):

https://codereview.chromium.org/1724643002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/uncond_br.ll:16: ; <label>:target                      
             ; preds = %2, %0
Just remove this comment.

[rkotlerimgtec, 2016-02-24 00:34:27]

https://codereview.chromium.org/1724643002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/uncond_br.ll (right):

https://codereview.chromium.org/1724643002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/uncond_br.ll:16: ; <label>:target                      
             ; preds = %2, %0
On 2016/02/24 00:11:54, stichnot wrote:
> Just remove this comment.

Done.

[Jim Stichnoth, 2016-02-24 02:59:33]

lgtm

[Jim Stichnoth, 2016-02-24 02:59:46]

Description was changed from

==========
Subzero: Basic unconditional branch for Mips.

This is part of ARM patch:
https://codereview.chromium.org/1151663004/

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

to

==========
Subzero: Basic unconditional branch for Mips.

This is part of ARM patch:
https://codereview.chromium.org/1151663004/

BUG=
R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-02-24 02:59:48]

Committed patchset #3 (id:40001) manually as
3fe4b570fe0c10e316e66a232b13e1b37a6d9614 (presubmit successful).