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

Issue 1216933015: X8632 Templatization completed. (Closed)

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

X8632 Templatization completed. This CL introduces the X86Inst templates. The previous implementation relied on template specialization which did not played nice with the new design. This required a lot of other boilerplate code (i.e., tons of new named constructors, one for each X86Inst.) This CL also moves X8632 code out of the X86Base{Impl}?.h files so that they are **almost** target agnostic. As we move to adding other X86 targets more methods will be moved to the target-specific trait class (e.g., call/ret/argument lowering.) BUG= https://code.google.com/p/nativeclient/issues/detail?id=4077 R=jvoung@chromium.org Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=921856d4e4bd4f0deedc7324d5f6a4ca0be3439f

Patch Set 1 #

Patch Set 2 : Removes almost all references to X8632 from the *Base.h and *BaseImpl.h files. #

Patch Set 3 : Eliminates all references to X8632 from Impl files. #

Total comments: 28

Patch Set 4 : Addresses comments. #

Total comments: 24

Patch Set 5 : Addresses comments. #

Patch Set 6 : Renames InstX86BaseFoo to InstX86Foo when that class is not expected to be derived from && make for… #

Patch Set 7 : git pull && merge #

Total comments: 15

Patch Set 8 : Addresses comments. #

Total comments: 6

Patch Set 9 : Addresses comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7181 lines, -5162 lines) Patch
M src/IceAssemblerX8632.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M src/IceAssemblerX86Base.h View 1 chunk +0 lines, -4 lines 0 comments Download
M src/IceInstX8632.h View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -1769 lines 0 comments Download
M src/IceInstX8632.cpp View 1 2 3 4 5 6 6 chunks +96 lines, -2896 lines 0 comments Download
A src/IceInstX86Base.h View 1 2 3 4 5 6 7 8 1 chunk +3148 lines, -0 lines 0 comments Download
A src/IceInstX86BaseImpl.h View 1 2 3 4 5 6 7 1 chunk +3162 lines, -0 lines 0 comments Download
M src/IceTargetLoweringX8632.cpp View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M src/IceTargetLoweringX8632Traits.h View 1 2 3 4 5 6 7 9 chunks +405 lines, -55 lines 0 comments Download
M src/IceTargetLoweringX86Base.h View 1 2 3 4 5 6 8 chunks +121 lines, -119 lines 0 comments Download
M src/IceTargetLoweringX86BaseImpl.h View 1 2 3 4 5 6 86 chunks +230 lines, -319 lines 0 comments Download

Messages

Total messages: 11 (1 generated)
John
5 years, 5 months ago (2015-07-06 16:31:57 UTC) #2
jvoung (off chromium)
https://codereview.chromium.org/1216933015/diff/30001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/1216933015/diff/30001/src/IceInstX8632.cpp#newcode10 src/IceInstX8632.cpp:10: // This file implements the InstX8632 and OperandX8632 classes, ...
5 years, 5 months ago (2015-07-06 18:58:46 UTC) #3
John
https://codereview.chromium.org/1216933015/diff/30001/src/IceInstX8632.cpp File src/IceInstX8632.cpp (right): https://codereview.chromium.org/1216933015/diff/30001/src/IceInstX8632.cpp#newcode10 src/IceInstX8632.cpp:10: // This file implements the InstX8632 and OperandX8632 classes, ...
5 years, 5 months ago (2015-07-06 22:30:09 UTC) #4
jvoung (off chromium)
https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX8632.h#newcode22 src/IceInstX8632.h:22: // instead of the old In general, I'm not ...
5 years, 5 months ago (2015-07-07 00:00:18 UTC) #5
John
https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX8632.h#newcode22 src/IceInstX8632.h:22: // instead of the old On 2015/07/07 00:00:17, jvoung ...
5 years, 5 months ago (2015-07-07 15:12:17 UTC) #6
jvoung (off chromium)
https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX8632.h#newcode12 src/IceInstX8632.h:12: /// modified I don't know why, but the line-wrapping ...
5 years, 5 months ago (2015-07-07 17:23:00 UTC) #7
John
https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX8632.h#newcode12 src/IceInstX8632.h:12: /// modified On 2015/07/07 17:22:59, jvoung wrote: > I ...
5 years, 5 months ago (2015-07-07 18:23:18 UTC) #8
jvoung (off chromium)
Thanks, otherwise LGTM https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX8632.h#newcode12 src/IceInstX8632.h:12: /// modified On 2015/07/07 18:23:17, John ...
5 years, 5 months ago (2015-07-07 18:53:11 UTC) #9
John
Thanks for the review. https://codereview.chromium.org/1216933015/diff/130001/src/IceInstX8632.h File src/IceInstX8632.h (right): https://codereview.chromium.org/1216933015/diff/130001/src/IceInstX8632.h#newcode16 src/IceInstX8632.h:16: /// X8632 On 2015/07/07 18:53:11, ...
5 years, 5 months ago (2015-07-07 18:56:21 UTC) #10
John
5 years, 5 months ago (2015-07-07 18:56:32 UTC) #11
Message was sent while issue was closed.
Committed patchset #9 (id:150001) manually as
921856d4e4bd4f0deedc7324d5f6a4ca0be3439f (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698