Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(563)

Issue 1176133004: implement the null function for the Mips32 subzero compiler (Closed)

Created:
5 years, 6 months ago by reed.kotler
Modified:
5 years, 5 months ago
CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : Run clang format #

Total comments: 16

Patch Set 3 : Fix patch to synch to master #

Total comments: 59

Patch Set 4 : Address code review comments #

Total comments: 5

Patch Set 5 : git diff #

Patch Set 6 : Fixing patch per review comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+235 lines, -81 lines) Patch
M Makefile.standalone View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M pydir/run-pnacl-sz.py View 1 2 2 chunks +7 lines, -3 lines 0 comments Download
M src/IceAssemblerMIPS32.h View 1 2 3 4 1 chunk +4 lines, -2 lines 0 comments Download
M src/IceInstMIPS32.h View 1 2 3 4 1 chunk +55 lines, -1 line 0 comments Download
A src/IceInstMIPS32.cpp View 1 2 3 4 1 chunk +62 lines, -0 lines 0 comments Download
M src/IceInstMIPS32.def View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/IceTargetLoweringMIPS32.h View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M src/IceTargetLoweringMIPS32.cpp View 1 2 3 4 15 chunks +90 lines, -74 lines 0 comments Download
M tests_lit/llvm2ice_tests/function_aligned.ll View 1 2 3 4 5 3 chunks +12 lines, -0 lines 1 comment Download

Messages

Total messages: 20 (1 generated)
reed.kotler
Implement the null function in Mips subzero. When running make Makefile.standalone format, it changed a ...
5 years, 6 months ago (2015-06-11 22:33:40 UTC) #2
Jim Stichnoth
Reed, BTW, do you have a link to some good public reference for the MIPS ...
5 years, 6 months ago (2015-06-12 18:03:14 UTC) #3
jvoung (off chromium)
https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h File src/IceAssemblerMIPS32.h (right): https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h#newcode53 src/IceAssemblerMIPS32.h:53: // TBD (Reed Kotler ) . Find out what ...
5 years, 6 months ago (2015-06-12 19:39:13 UTC) #4
reed.kotler
On 2015/06/12 19:39:13, jvoung wrote: > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h > File src/IceAssemblerMIPS32.h (right): > > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h#newcode53 > ...
5 years, 6 months ago (2015-06-12 20:01:33 UTC) #5
reed.kotler
On 2015/06/12 20:01:33, reed.kotler wrote: > On 2015/06/12 19:39:13, jvoung wrote: > > https://codereview.chromium.org/1176133004/diff/20001/src/IceAssemblerMIPS32.h > ...
5 years, 5 months ago (2015-07-07 00:22:13 UTC) #6
reed.kotler
On 2015/06/12 18:03:14, stichnot wrote: > Reed, BTW, do you have a link to some ...
5 years, 5 months ago (2015-07-07 00:37:05 UTC) #7
jvoung (off chromium)
Welcome back When review comments are addressed, you should be able to click on them ...
5 years, 5 months ago (2015-07-07 18:02:35 UTC) #8
reed.kotler
On 2015/07/07 18:02:35, jvoung wrote: > Welcome back > > When review comments are addressed, ...
5 years, 5 months ago (2015-07-07 20:45:25 UTC) #9
jvoung (off chromium)
On 2015/07/07 20:45:25, reed.kotler wrote: > On 2015/07/07 18:02:35, jvoung wrote: > > Welcome back ...
5 years, 5 months ago (2015-07-07 21:15:52 UTC) #10
reed.kotler
Forgot to publish the comments. Lol https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py File pydir/run-pnacl-sz.py (right): https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#newcode22 pydir/run-pnacl-sz.py:22: 'mips32': ['-triple=mipsel-none-nacl' ] ...
5 years, 5 months ago (2015-07-07 21:54:55 UTC) #11
jvoung (off chromium)
Sorry for being so nitpicky about code style, but we try to be consistent. A ...
5 years, 5 months ago (2015-07-08 00:39:12 UTC) #12
reed.kotler
Sorry. There were two files that did not get saved in Eclipse for some reason. ...
5 years, 5 months ago (2015-07-08 01:05:29 UTC) #13
reed.kotler
Hopefully I got all these formatting changes this time. https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py File pydir/run-pnacl-sz.py (right): https://codereview.chromium.org/1176133004/diff/40001/pydir/run-pnacl-sz.py#newcode22 pydir/run-pnacl-sz.py:22: ...
5 years, 5 months ago (2015-07-08 02:05:17 UTC) #14
jvoung (off chromium)
LGTM
5 years, 5 months ago (2015-07-08 16:10:09 UTC) #15
jvoung (off chromium)
https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tests/function_aligned.ll File tests_lit/llvm2ice_tests/function_aligned.ll (right): https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tests/function_aligned.ll#newcode34 tests_lit/llvm2ice_tests/function_aligned.ll:34: ; MIPS32: 4: {{.*}} jr ra About "Is it ...
5 years, 5 months ago (2015-07-08 16:44:26 UTC) #16
jvoung (off chromium)
Committed patchset #6 (id:100001) manually as d00d48da12ac44879066724978a032be64fcb0ad (presubmit successful).
5 years, 5 months ago (2015-07-08 16:49:12 UTC) #17
reed.kotler
On 2015/07/08 16:44:26, jvoung wrote: > https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tests/function_aligned.ll > File tests_lit/llvm2ice_tests/function_aligned.ll (right): > > https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tests/function_aligned.ll#newcode34 > ...
5 years, 5 months ago (2015-07-09 19:18:10 UTC) #18
reed.kotler
I would like to resolve this target issue with -none-acl . Am requesting clarification on ...
5 years, 5 months ago (2015-07-09 19:22:52 UTC) #19
jvoung (off chromium)
5 years, 5 months ago (2015-07-09 19:25:03 UTC) #20
Message was sent while issue was closed.
On 2015/07/09 19:18:10, reed.kotler wrote:
> On 2015/07/08 16:44:26, jvoung wrote:
> >
>
https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes...
> > File tests_lit/llvm2ice_tests/function_aligned.ll (right):
> > 
> >
>
https://codereview.chromium.org/1176133004/diff/100001/tests_lit/llvm2ice_tes...
> > tests_lit/llvm2ice_tests/function_aligned.ll:34: ; MIPS32: 4: {{.*}} jr ra
> > About "Is it possible to leave out the "-none-nacl" for now", the reason it
> > fails when you remove the "-none-nacl" is that this check is looking for "4:
> > {{.*}} jr ra". If it was starting at "0:" then that would work with
non-nacl.
> > 
> > When the "check-lit" fails, it will print a whole slew of commands to
> reproduce
> > the failure, and one of them is the mips invocation. Copy that and try it
> > with/without the "-none-nacl". E.g.,
> > 
> > diff temp.nonnacl temp.nacl                                                 
 
>  
> >                                                            
> (git)-[reeds_change]
> > 
> > 2c2
> > < /tmp/tmpRYvxTG:     file format elf32-tradlittlemips-nacl
> > ---
> > > /tmp/tmp7nrsu2:     file format elf32-tradlittlemips-nacl
> > 8,10c8,10
> > <    0:	03e00008 	jr	ra
> > <    4:	00000000 	nop
> > <    8:	e7fedef0 	swc1	$f30,-8464(ra)
> > ---
> > >    0:	03eef824 	and	ra,ra,t6
> > >    4:	03e00008 	jr	ra
> > >    8:	00000000 	nop
> > 14,15c14,16
> > <   10:	03e00008 	jr	ra
> > <   14:	00000000 	nop
> > ---
> > >   10:	03eef824 	and	ra,ra,t6
> > >   14:	03e00008 	jr	ra
> > >   18:	00000000 	nop
> > 
> > The nacl version has the masking for ra prior to the jump, which is why it
is
> at
> > 4: instead of at 0:. Unlike ARM and X86, the LLVM assembler for MIPS applies
> > "auto-sandboxing". That is, besides inserting nops for alignment, it can
also
> > insert masking instructions (the ARM and X86 assemblers don't do that yet...
> > since it can often involve a free register or reserving a register, it does
> > require some cooperation with the compiler).
> > 
> > Anyway, I'll help commit this as is, and you can adjust this in the next
> change,
> > if you agree with the assessment.
> 
> What is your proposal?
> 
> I'm new to NACL/PNACL and am not conversant on the issues here.
> 
> You want me to make that arch just be Mips and fix the offset in the test?

Yes, my proposal is to remove the -none-nacl to be consistent with the rest, and
fix the offsets in the test.

pnacl-sz has a separate option to "--sandbox" and it should only do the
equivalent of "-none-nacl" when that argument is given.

Powered by Google App Engine
This is Rietveld 408576698