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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2368343003: Subzero, MIPS32: Intrinsic call Bswap for i16, i32 and i64 (Closed)

Created:
4 years, 2 months ago by obucinac
Modified:
4 years, 2 months ago
CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Subzero, MIPS32: Intrinsic call Bswap for i16, i32 and i64 Implements intrinsic call llvm.bswap for i16, i32 and i64 R=stichnot@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=175cb1381a25d5fdd961bfeab4dea0dfb92a527a

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressing review comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+616 lines, -1 line) Patch
M src/IceInstMIPS32.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/IceTargetLoweringMIPS32.cpp View 1 1 chunk +90 lines, -1 line 1 comment Download
M tests_lit/assembler/mips32/encoding_intrinsics.ll View 1 1 chunk +466 lines, -0 lines 0 comments Download
M tests_lit/llvm2ice_tests/nacl-other-intrinsics.ll View 1 4 chunks +59 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (3 generated)
obucinac
Shares some changes with https://codereview.chromium.org/2364093002/ to enable encoding test to run properly. Whichever lands first, ...
4 years, 2 months ago (2016-09-26 19:15:12 UTC) #3
Jim Stichnoth
https://codereview.chromium.org/2368343003/diff/1/src/IceInstMIPS32.cpp File src/IceInstMIPS32.cpp (right): https://codereview.chromium.org/2368343003/diff/1/src/IceInstMIPS32.cpp#newcode903 src/IceInstMIPS32.cpp:903: template <> void InstMIPS32Lui::emitIAS(const Cfg *Func) const { This ...
4 years, 2 months ago (2016-09-27 04:20:32 UTC) #4
Jim Stichnoth
lgtm https://codereview.chromium.org/2368343003/diff/20001/src/IceTargetLoweringMIPS32.cpp File src/IceTargetLoweringMIPS32.cpp (right): https://codereview.chromium.org/2368343003/diff/20001/src/IceTargetLoweringMIPS32.cpp#newcode3119 src/IceTargetLoweringMIPS32.cpp:3119: _andi(T3, T3, 65280); this magic constant too (I ...
4 years, 2 months ago (2016-09-27 13:59:53 UTC) #5
Jim Stichnoth
Committed patchset #2 (id:20001) manually as 175cb1381a25d5fdd961bfeab4dea0dfb92a527a (presubmit successful).
4 years, 2 months ago (2016-09-27 14:00:29 UTC) #7
obucinac
4 years, 2 months ago (2016-09-27 14:22:02 UTC) #8
Message was sent while issue was closed. 
https://codereview.chromium.org/2368343003/diff/1/src/IceInstMIPS32.cpp
File src/IceInstMIPS32.cpp (right):

https://codereview.chromium.org/2368343003/diff/1/src/IceInstMIPS32.cpp#newco...
src/IceInstMIPS32.cpp:903: template <> void InstMIPS32Lui::emitIAS(const Cfg
*Func) const {
On 2016/09/27 04:20:32, stichnot wrote:
> This specialization also needs to be declared in the .h file.

Done.

https://codereview.chromium.org/2368343003/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2368343003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3037: break;
On 2016/09/27 04:20:32, stichnot wrote:
> Can this "break" be changed to "return"?  That would make the code a bit
easier
> to follow.  (here and below)

Done.

https://codereview.chromium.org/2368343003/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:3048: _andi(T2, T2, 65280);
On 2016/09/27 04:20:32, stichnot wrote:
> Maybe this strange-looking constant would be more understandable if written in
> hex form?

Done.

Powered by Google App Engine
This is Rietveld 408576698