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

Issue 1670413002: Add NOP to ARM IR lowering. (Closed)

Created:
4 years, 10 months ago by Karl
Modified:
4 years, 10 months ago
CC:
native-client-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/native_client/pnacl-subzero.git@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Add NOP to ARM IR lowering. This change allows pnacl-sz to randomly insert NOPs into the generated code, as is already done with X86. BUG=None R=eholk@chromium.org, stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f084a57174a1fe74522c8d41cffe578b3f11ae84

Patch Set 1 #

Patch Set 2 : Add test case. #

Total comments: 12

Patch Set 3 : Fix nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+117 lines, -4 lines) Patch
M src/IceInstARM32.h View 1 2 2 chunks +19 lines, -0 lines 0 comments Download
M src/IceInstARM32.cpp View 1 2 3 chunks +26 lines, -3 lines 0 comments Download
M src/IceTargetLoweringARM32.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M src/IceTargetLoweringARM32.cpp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M tests_lit/llvm2ice_tests/nop-insertion.ll View 1 2 2 chunks +69 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
Karl
4 years, 10 months ago (2016-02-05 21:53:13 UTC) #3
Karl
4 years, 10 months ago (2016-02-08 17:13:47 UTC) #4
Eric Holk
https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h File src/IceInstARM32.h (right): https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h#newcode1438 src/IceInstARM32.h:1438: class InstARM32Nop final : public InstARM32Pred { Does it ...
4 years, 10 months ago (2016-02-08 17:29:43 UTC) #5
Karl
https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h File src/IceInstARM32.h (right): https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h#newcode1438 src/IceInstARM32.h:1438: class InstARM32Nop final : public InstARM32Pred { On 2016/02/08 ...
4 years, 10 months ago (2016-02-08 17:55:55 UTC) #6
Eric Holk
lgtm https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h File src/IceInstARM32.h (right): https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h#newcode1438 src/IceInstARM32.h:1438: class InstARM32Nop final : public InstARM32Pred { On ...
4 years, 10 months ago (2016-02-08 17:58:15 UTC) #7
Jim Stichnoth
lgtm https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp File src/IceInstARM32.cpp (right): https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp#newcode1097 src/IceInstARM32.cpp:1097: InstARM32Nop::InstARM32Nop(Cfg *Func) Should nop (and dmb above) be ...
4 years, 10 months ago (2016-02-08 18:17:51 UTC) #8
Karl
Committed patchset #3 (id:40001) manually as f084a57174a1fe74522c8d41cffe578b3f11ae84 (presubmit successful).
4 years, 10 months ago (2016-02-09 21:09:27 UTC) #10
Karl
https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp File src/IceInstARM32.cpp (right): https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp#newcode1097 src/IceInstARM32.cpp:1097: InstARM32Nop::InstARM32Nop(Cfg *Func) On 2016/02/08 18:17:50, stichnot wrote: > Should ...
4 years, 10 months ago (2016-02-09 21:10:02 UTC) #11
Jim Stichnoth
4 years, 10 months ago (2016-02-09 21:22:40 UTC) #12
Message was sent while issue was closed.
> On 2016/02/08 18:17:50, stichnot wrote:
> > All these def.pseudo lines seem to have a trailing space character.
> > 
> > (I guess FakeDef's emit routine ought to be fixed...)
> 
> Removed spaces. Fixing the dump() method for FakeDef is harder. It calls
> emitSources() which doesn't add a space. Will look into fixing this in another
> CL.

FYI, my pending CL takes care of this, so need to do it.

Powered by Google App Engine
This is Rietveld 408576698