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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1414883007: Subzero. ARM32. Implements bool folding. (Closed)

Created:
5 years, 1 month ago by John
Modified:
5 years, 1 month ago
Reviewers:
Jim Stichnoth, Karl, sehr
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 the BoolProducerTracker. #

Patch Set 3 : Icmp folding, handle BR #

Patch Set 4 : Handle SELECT #

Patch Set 5 : Minor changes #

Patch Set 6 : Minor lowerFcmp refactor, in preparation for BoolFolding it. #

Patch Set 7 : Fcmp folding. #

Patch Set 8 : Trunc folding. #

Patch Set 9 : Handles Zext #

Patch Set 10 : Handles Sext. #

Patch Set 11 : Refactors the code. #

Patch Set 12 : Moar refactoring. #

Patch Set 13 : Fixes lit tests. #

Total comments: 14

Patch Set 14 : Addresses comments && pulls. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+824 lines, -374 lines) Patch
M src/IceTargetLoweringARM32.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +125 lines, -2 lines 0 comments Download
M src/IceTargetLoweringARM32.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 16 chunks +394 lines, -102 lines 0 comments Download
M tests_lit/assembler/arm32/branch-mult-fwd.ll View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +16 lines, -16 lines 0 comments Download
M tests_lit/llvm2ice_tests/64bit.pnacl.ll View 1 2 3 4 5 6 7 8 9 10 11 12 23 chunks +96 lines, -59 lines 0 comments Download
M tests_lit/llvm2ice_tests/bool-folding.ll View 1 2 3 4 5 6 7 8 9 10 11 12 10 chunks +61 lines, -75 lines 0 comments Download
M tests_lit/llvm2ice_tests/branch-opt.ll View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +6 lines, -11 lines 0 comments Download
M tests_lit/llvm2ice_tests/fp.cmp.ll View 1 2 3 4 5 6 7 8 9 10 11 12 41 chunks +75 lines, -64 lines 0 comments Download
M tests_lit/llvm2ice_tests/select-opt.ll View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +12 lines, -8 lines 0 comments Download
M tests_lit/llvm2ice_tests/test_i1.ll View 1 2 3 4 5 6 7 8 9 10 11 12 10 chunks +39 lines, -37 lines 0 comments Download

Messages

Total messages: 5 (1 generated)
John
5 years, 1 month ago (2015-11-03 00:09:09 UTC) #2
Jim Stichnoth
Several nits, but otherwise lgtm https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringARM32.cpp File src/IceTargetLoweringARM32.cpp (right): https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringARM32.cpp#newcode1824 src/IceTargetLoweringARM32.cpp:1824: Constant *Zero = Ctx->getConstantZero(IceType_i32); ...
5 years, 1 month ago (2015-11-03 20:04:28 UTC) #3
John
Committed patchset #14 (id:260001) manually as 4a5e6d05151ba1b52f1425ff22aa0398328b89f7 (presubmit successful).
5 years, 1 month ago (2015-11-04 17:33:00 UTC) #4
John
5 years, 1 month ago (2015-11-05 20:25:39 UTC) #5
Message was sent while issue was closed. 
https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:1824: Constant *Zero =
Ctx->getConstantZero(IceType_i32);
On 2015/11/03 20:04:28, stichnot wrote:
> Use "_0" for consistency?

Done.

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2669: Constant *Zero =
Ctx->getConstantZero(IceType_i32);
On 2015/11/03 20:04:28, stichnot wrote:
> _0 and _1 ?

Done.

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:3967: *CondIfTrue = CondARM32::NE;  /* NE <->
APSR.Z == 0 */
On 2015/11/03 20:04:28, stichnot wrote:
> Why the /* */ style comments?

I don't know. Done.

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:331: // _mov_i1_to_flags is used for bool folding.
If "Boolean" is fold, this
On 2015/11/03 20:04:28, stichnot wrote:
> There are a few places in comments where I think "fold" might be more properly
> "folded" - take a second look and verify?

Done.

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:339: // _cmov is a pseudo instruction that is used
fol boolean folding. It emits
On 2015/11/03 20:04:28, stichnot wrote:
> for

Done.

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:650: struct BoolComputationEntry {
On 2015/11/03 20:04:28, stichnot wrote:
> Make this class instead of struct?
> 
> I'm not sure what various style guides have to say on the matter, but in
Subzero
> we've generally used "class" whenever there are actual methods defined,
> including ctor/dtor.  Most if not all of the exceptions are from following
> "standard" STL documentation for defining functors, or allocators, or whatnot.

I tend to think of classes as "data with behavior," and structs and "glorified
PODs." I also think "how hard would it be to replace this type with an
std::tuple<>?" If it is trivial, than the type is a struct; otherwise is a
class. Done anyway.

https://codereview.chromium.org/1414883007/diff/240001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:652: Inst *Instr;
On 2015/11/03 20:04:28, stichnot wrote:
> How about
>   Inst *const Instr;
> ?

Done.

Powered by Google App Engine
This is Rietveld 408576698