Issue 2051713002: [Subzero][MIPS32] Adds prolog instructions for MIPS32

Issue 2051713002: [Subzero][MIPS32] Adds prolog instructions for MIPS32 (Closed)

Created:
4 years, 6 months ago by sagar.thakur

Modified:
4 years, 6 months ago

Reviewers:
Eric Holk, Karl, Jim Stichnoth, John

CC:
native-client-reviews_googlegroups.com, jaydeep.patil, rich.fuhler_imgtec.com, mohit.bhakkad, srdjan.obucina_imgtec.com

Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

Description

[Subzero][MIPS32] Adds prolog instructions for MIPS32 BUG=none R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=a49fce0b53867c73e7d94d2591c96a4aa521dd7d

Patch Set 2 : Addressed review comments #

Patch Set 3 : Changes related to calling convention #

Total comments: 1

Created: 4 years, 6 months ago

	Unified diffs	Side-by-side diffs	Stats (+318 lines, -5 lines)
M	src/IceInstMIPS32.def	View	1 chunk	+1 line, -1 line	0 comments
M	src/IceTargetLoweringMIPS32.h	View	3 chunks	+13 lines, -0 lines	0 comments
M	src/IceTargetLoweringMIPS32.cpp	View	5 chunks	+302 lines, -2 lines	1 comment
M	tests_lit/llvm2ice_tests/uncond_br.ll	View	1 chunk	+2 lines, -2 lines	0 comments

Messages

Total messages: 10 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

sagar.thakur

Patch dependent on https://codereview.chromium.org/2021033002/ for CallingConv, applyStackAlignment, MIPS32_STACK_ALIGNMENT_BYTES etc. Some part of prolog instruction emission ...

4 years, 6 months ago (2016-06-09 08:03:26 UTC) #2

Jim Stichnoth

This looks OK, except that as mentioned it also needs https://codereview.chromium.org/2021033002/ to be applied in ...

4 years, 6 months ago (2016-06-09 21:19:47 UTC) #3

sagar.thakur

On 2016/06/09 21:19:47, stichnot wrote: > This looks OK, except that as mentioned it also ...

4 years, 6 months ago (2016-06-10 09:39:48 UTC) #4

sagar.thakur

https://codereview.chromium.org/2051713002/diff/1/src/IceTargetLoweringMIPS32.cpp File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2051713002/diff/1/src/IceTargetLoweringMIPS32.cpp#newcode743 src/IceTargetLoweringMIPS32.cpp:743: SpillAreaSizeBytes, GlobalsAndSubsequentPaddingSize, UsesFramePointer); On 2016/06/09 21:19:47, stichnot wrote: > ...

4 years, 6 months ago (2016-06-10 09:40:35 UTC) #5

Jim Stichnoth

On 2016/06/10 09:39:48, sagar.thakur wrote: > On 2016/06/09 21:19:47, stichnot wrote: > > This looks ...

4 years, 6 months ago (2016-06-10 23:07:51 UTC) #6

On 2016/06/10 09:39:48, sagar.thakur wrote:
> On 2016/06/09 21:19:47, stichnot wrote:
> > This looks OK, except that as mentioned it also needs
> > https://codereview.chromium.org/2021033002/ to be applied in order to
compile,
> > and even then "make check-lit" still fails with (I think) liveness
validation
> > errors.
> > 
> > I would be OK committing several "broken" CLs within a short time window as
> long
> > as the end result passes the presubmit checks.
> > 
> >
>
https://codereview.chromium.org/2051713002/diff/1/src/IceTargetLoweringMIPS32...
> > File src/IceTargetLoweringMIPS32.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2051713002/diff/1/src/IceTargetLoweringMIPS32...
> > src/IceTargetLoweringMIPS32.cpp:743: SpillAreaSizeBytes,
> > GlobalsAndSubsequentPaddingSize, UsesFramePointer);
> > 80-col
> > 
> >
>
https://codereview.chromium.org/2051713002/diff/1/tests_lit/llvm2ice_tests/un...
> > File tests_lit/llvm2ice_tests/uncond_br.ll (right):
> > 
> >
>
https://codereview.chromium.org/2051713002/diff/1/tests_lit/llvm2ice_tests/un...
> > tests_lit/llvm2ice_tests/uncond_br.ll:26: ; MIPS32:   b  {{[0-9 | a-f]+}}
> > <.Luncond1$target>
> > I think the regex should just be
> >   [0-9a-f]+
> > Right?
> > (here and below)
> 
> I have added the function applyStackAlignmentTy and the variable
> MIPS32_STACK_ALIGNMENT_BYTES required for this patch. The CallingConv class
also
> required for this patch is in a separate issue
> (https://codereview.chromium.org/2052793003/) which should be commited before
> this patch is committed.

I tried applying this CL and https://codereview.chromium.org/2052793003/
together, but the result does not compile:

src/IceTargetLoweringMIPS32.cpp:903:12: error: 'argInGPR' is a private member of
'Ice::MIPS32::TargetMIPS32::CallingConv'
    if (CC.argInGPR(Ty, &DummyReg)) {
           ^
src/IceTargetLoweringMIPS32.h:489:10: note: declared private here
    bool argInGPR(Type Ty, RegNumT *Reg);
         ^
1 error generated.

sagar.thakur

On 2016/06/10 23:07:51, stichnot wrote: > On 2016/06/10 09:39:48, sagar.thakur wrote: > > On 2016/06/09 ...

4 years, 6 months ago (2016-06-13 07:45:53 UTC) #7

On 2016/06/10 23:07:51, stichnot wrote:
> On 2016/06/10 09:39:48, sagar.thakur wrote:
> > On 2016/06/09 21:19:47, stichnot wrote:
> > > This looks OK, except that as mentioned it also needs
> > > https://codereview.chromium.org/2021033002/ to be applied in order to
> compile,
> > > and even then "make check-lit" still fails with (I think) liveness
> validation
> > > errors.
> > > 
> > > I would be OK committing several "broken" CLs within a short time window
as
> > long
> > > as the end result passes the presubmit checks.
> > > 
> > >
> >
>
https://codereview.chromium.org/2051713002/diff/1/src/IceTargetLoweringMIPS32...
> > > File src/IceTargetLoweringMIPS32.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2051713002/diff/1/src/IceTargetLoweringMIPS32...
> > > src/IceTargetLoweringMIPS32.cpp:743: SpillAreaSizeBytes,
> > > GlobalsAndSubsequentPaddingSize, UsesFramePointer);
> > > 80-col
> > > 
> > >
> >
>
https://codereview.chromium.org/2051713002/diff/1/tests_lit/llvm2ice_tests/un...
> > > File tests_lit/llvm2ice_tests/uncond_br.ll (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2051713002/diff/1/tests_lit/llvm2ice_tests/un...
> > > tests_lit/llvm2ice_tests/uncond_br.ll:26: ; MIPS32:   b  {{[0-9 | a-f]+}}
> > > <.Luncond1$target>
> > > I think the regex should just be
> > >   [0-9a-f]+
> > > Right?
> > > (here and below)
> > 
> > I have added the function applyStackAlignmentTy and the variable
> > MIPS32_STACK_ALIGNMENT_BYTES required for this patch. The CallingConv class
> also
> > required for this patch is in a separate issue
> > (https://codereview.chromium.org/2052793003/) which should be commited
before
> > this patch is committed.
> 
> I tried applying this CL and https://codereview.chromium.org/2052793003/
> together, but the result does not compile:
> 
> src/IceTargetLoweringMIPS32.cpp:903:12: error: 'argInGPR' is a private member
of
> 'Ice::MIPS32::TargetMIPS32::CallingConv'
>     if (CC.argInGPR(Ty, &DummyReg)) {
>            ^
> src/IceTargetLoweringMIPS32.h:489:10: note: declared private here
>     bool argInGPR(Type Ty, RegNumT *Reg);
>          ^
> 1 error generated.

Hi Jim,

This error occurred because the patch for CallingConv
(https://codereview.chromium.org/2052793003/) was recently changed. The function
argInGPR in CallingConv was changed to argInReg. I have updated this patch to
reflect the same. With this change it builds without any error and with all
tests passing.

Jim Stichnoth

lgtm https://codereview.chromium.org/2051713002/diff/40001/src/IceTargetLoweringMIPS32.cpp File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2051713002/diff/40001/src/IceTargetLoweringMIPS32.cpp#newcode79 src/IceTargetLoweringMIPS32.cpp:79: constexpr uint32_t MIPS32_STACK_ALIGNMENT_BYTES = 8; It turns out ...

4 years, 6 months ago (2016-06-13 12:53:31 UTC) #8

Jim Stichnoth

4 years, 6 months ago (2016-06-13 12:55:06 UTC) #10

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages