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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1343783003: Subzero. Implements the scalar bitcast operations for ARM32. (Closed)

Created:
5 years, 3 months ago by John
Modified:
5 years, 3 months ago
Reviewers:
Jim Stichnoth
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

Patch Set 1 #

Patch Set 2 : Implements Bitcast lowering. #

Patch Set 3 : Removes an else because LLVM style. #

Patch Set 4 : Disable ARM32 tests in MINIMAL=1 build." #

Patch Set 5 : Removes superfluous TODOs. #

Total comments: 8

Patch Set 6 : Addresses comments. #

Patch Set 7 : Addresses comments, and merges. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+297 lines, -17 lines) Patch
M src/IceInstARM32.h View 1 2 3 4 5 6 3 chunks +87 lines, -2 lines 0 comments Download
M src/IceInstARM32.cpp View 1 2 3 4 5 3 chunks +75 lines, -6 lines 0 comments Download
M src/IceTargetLoweringARM32.h View 1 2 3 4 5 6 1 chunk +13 lines, -0 lines 0 comments Download
M src/IceTargetLoweringARM32.cpp View 1 2 3 4 5 3 chunks +78 lines, -2 lines 0 comments Download
M tests_lit/llvm2ice_tests/bitcast.ll View 1 2 3 7 chunks +28 lines, -0 lines 0 comments Download
M tests_lit/llvm2ice_tests/fp.convert.ll View 1 2 3 5 chunks +16 lines, -7 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
John
5 years, 3 months ago (2015-09-14 20:06:27 UTC) #2
Jim Stichnoth
otherwise lgtm https://codereview.chromium.org/1343783003/diff/80001/src/IceInstARM32.cpp File src/IceInstARM32.cpp (right): https://codereview.chromium.org/1343783003/diff/80001/src/IceInstARM32.cpp#newcode507 src/IceInstARM32.cpp:507: assert(llvm::dyn_cast<OperandARM32Mem>(Src0) == nullptr); nit: use !isa instead ...
5 years, 3 months ago (2015-09-14 23:15:42 UTC) #3
John
Committed patchset #7 (id:120001) manually as f977f7157088e2fd3de41c2acf4c62bfd655f15a (presubmit successful).
5 years, 3 months ago (2015-09-14 23:28:37 UTC) #4
John
5 years, 3 months ago (2015-09-16 21:07:52 UTC) #5
Message was sent while issue was closed. 
https://codereview.chromium.org/1343783003/diff/80001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1343783003/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:507: assert(llvm::dyn_cast<OperandARM32Mem>(Src0) ==
nullptr);
On 2015/09/14 23:15:42, stichnot wrote:
> nit: use !isa instead of dyn_cast

Done.

https://codereview.chromium.org/1343783003/diff/80001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:540: Ostream &Str = Func->getContext()->getStrEmit();
On 2015/09/14 23:15:42, stichnot wrote:
> Use "if (!BuildDefs::dump()) return;" to ensure this is an empty function in
> minimal mode.

I would hope that, in minimal build, the linker gets rid of this function (note
that this is a private function that is only invoked in non-MINIMAL=1 build.)
Done.

https://codereview.chromium.org/1343783003/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1343783003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:2152: // avoid cryptic livebess errors
On 2015/09/14 23:15:42, stichnot wrote:
> liveness

Done.

https://codereview.chromium.org/1343783003/diff/80001/src/IceTargetLoweringAR...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1343783003/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.h:413: // This represents the simple single source,
single dest variants only.
On 2015/09/14 23:15:42, stichnot wrote:
> Update this comment?

Done.

Powered by Google App Engine
This is Rietveld 408576698