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

Issue 1457683004: Subzero. ARM32. Removing memory legalization warts. (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

Subzero. ARM32. Removes memory legalization warts. This CL removes two warts from the ARM32 backend: 1) during argument lowering, if a stack parameter is assigned a register, the backend creates a new Variable that references the stack location with the incoming argument, and _mov() it to the parameter. 2) During stack slot legalization, all _mov(Mem(), Reg) are converted to stores; and all _mov(Reg, Mem()) are converted to loads. BUG= https://code.google.com/p/nativeclient/issues/detail?id=4076 R=kschimpf@google.com Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=3f6b47d53174f4f44d8b7c32d806fc5b5288a218

Patch Set 1 #

Patch Set 2 : Legalizes _mov(Var, Mem()) and _mov(Mem(), Var) to _ldr and _str. #

Patch Set 3 : Fixes lit. presubmit cleared. #

Total comments: 6

Patch Set 4 : Addresses comments #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -180 lines) Patch
M src/IceInstARM32.cpp View 1 2 2 chunks +43 lines, -70 lines 0 comments Download
M src/IceTargetLoweringARM32.h View 1 2 4 chunks +26 lines, -15 lines 0 comments Download
M src/IceTargetLoweringARM32.cpp View 1 2 3 15 chunks +61 lines, -92 lines 2 comments Download
M tests_lit/assembler/arm32/branch-mult-fwd.ll View 1 2 3 chunks +3 lines, -1 line 2 comments Download
M tests_lit/assembler/arm32/load-store.ll View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M tests_lit/assembler/arm32/mov-const.ll View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M tests_lit/assembler/arm32/store-sf.ll View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 10 (4 generated)
John
5 years, 1 month ago (2015-11-18 17:49:40 UTC) #3
Karl
Otherwise, LGTM. https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringARM32.cpp File src/IceTargetLoweringARM32.cpp (right): https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringARM32.cpp#newcode478 src/IceTargetLoweringARM32.cpp:478: /* Remove comment? https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringARM32.cpp#newcode1029 src/IceTargetLoweringARM32.cpp:1029: auto *SrcR ...
5 years, 1 month ago (2015-11-18 18:40:42 UTC) #5
John
https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringARM32.cpp File src/IceTargetLoweringARM32.cpp (right): https://codereview.chromium.org/1457683004/diff/40001/src/IceTargetLoweringARM32.cpp#newcode478 src/IceTargetLoweringARM32.cpp:478: /* On 2015/11/18 18:40:42, Karl wrote: > Remove comment? ...
5 years, 1 month ago (2015-11-19 13:41:22 UTC) #7
John
Committed patchset #4 (id:80001) manually as 3f6b47d53174f4f44d8b7c32d806fc5b5288a218 (presubmit successful).
5 years, 1 month ago (2015-11-19 13:43:03 UTC) #8
Jim Stichnoth
necro lgtm https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringARM32.cpp File src/IceTargetLoweringARM32.cpp (right): https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringARM32.cpp#newcode1024 src/IceTargetLoweringARM32.cpp:1024: auto *const SrcR = llvm::cast<Variable>(Src); "auto *const" ...
5 years, 1 month ago (2015-11-22 03:40:47 UTC) #9
John
5 years, 1 month ago (2015-11-23 18:52:56 UTC) #10
Message was sent while issue was closed.
Done in another cl.

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

https://codereview.chromium.org/1457683004/diff/80001/src/IceTargetLoweringAR...
src/IceTargetLoweringARM32.cpp:1024: auto *const SrcR =
llvm::cast<Variable>(Src);
On 2015/11/22 03:40:47, stichnot wrote:
> "auto *const" seems odd -- did you mean "const auto *"?

No, I meant auto *const. Removing as git grep 'auto *const' reveals no other
usages.

https://codereview.chromium.org/1457683004/diff/80001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/branch-mult-fwd.ll (right):

https://codereview.chromium.org/1457683004/diff/80001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/branch-mult-fwd.ll:27: ; ASM-NEXT:     # [sp, #8] =
def.pseudo
On 2015/11/22 03:40:47, stichnot wrote:
> This makes me wonder - should we really be emitting such pseudo-instructions
in
> a comment?

Acknowledged.

Powered by Google App Engine
This is Rietveld 408576698