Issue 1216933015: X8632 Templatization completed.

Issue 1216933015: X8632 Templatization completed. (Closed)

Created:
5 years, 5 months ago by John

Modified:
5 years, 5 months ago

Reviewers:
jvoung (off chromium), Karl, Jim Stichnoth, JF

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.

More Reviews

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. #

Created: 5 years, 5 months ago

Download [raw] [tar.bz2]

	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)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

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

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

5 years, 5 months ago (2015-07-07 15:12:17 UTC) #6

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#newc...
src/IceInstX8632.h:22: // instead of the old
On 2015/07/07 00:00:17, jvoung wrote:
> In general, I'm not a huge fan of the historical-style comments, though this
> could be useful for us during a transition.
> 
> The bits about "definitions are in IceInstX86Base.h", "When interacting
> with...", "for example, to create an ..." and "In the future, this file might
be
> used to declare X8632 specific..." do seem useful though.

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:9: //
On 2015/07/07 00:00:18, jvoung wrote:
> Can you add the \file and /// for doxygen to this new file?
> 
> Sorry, you might find some rebase conflicts...

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceInstX86Base.h#ne...
src/IceInstX86Base.h:2737: using FakeRMW = InstX86BaseFakeRMW<Machine>;
On 2015/07/07 00:00:17, jvoung wrote:
> So just checking on this type aliasing, this can be template specialized so
that
> for <TargetX8664> struct Insts { ...  Ret = InstX8664Ret<TargetX8664> ... };
or
> some such, instead of Ret = InstX86BaseRet<Machine>? My template and c++11 foo
> is not so advanced =)
> 
> So the Base really does indicate something that can be overridden (vs just
> naming things without the "Base" like "InstX86Ret<Machine>").
> 
> Then the target lowering will refer to "Inst::Ret" and that will be the right
> one.

Yes, this template <> struct Insts is just a shortcut to avoid each backend
having to specialize each X86 instruction.

This comment made me realize that the "Base" annotations are just plain wrong
for these classes: they are not meant to be derived by clients, but rather
"specialized" for each machine. I went ahead and (1) removed the Base from their
names; and (2) annotated each class with the "final" keyword.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:408: // to reduce the amount of work  needed
for searching for opportunities.
On 2015/07/07 00:00:18, jvoung wrote:
> extra space between "work" and "needed"

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:584: Traits::Insts::FakeRMW::create(Func,
ArithSrcOther,
On 2015/07/07 00:00:18, jvoung wrote:
> I wonder if we should allow "auto" for results of ::create() functions like we
> do for dyn_cast<T>...
> 
> Anyway, could wait for Jim before deciding on this.

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:1178: if (typename Traits::X86OperandMem *Mem
=
On 2015/07/07 00:00:18, jvoung wrote:
> could declare as "auto *Mem" for this, since it's coming from dyn_cast ?

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:1208: if (typename Traits::X86OperandMem *Mem
=
On 2015/07/07 00:00:18, jvoung wrote:
> auto

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4223: // the source operand is transformed
into an typename Traits::X86OperandMem
On 2015/07/07 00:00:18, jvoung wrote:
> Could omit the "typename" within a comment? I suppose it makes search and
> replace harder though.

Yes, it makes sense. It did not occur to me to

/\/\/.*typename

after sed'ing.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4243: // other typename
Traits::X86OperandMem, so there should be no mention of
On 2015/07/07 00:00:18, jvoung wrote:
> similar, in a few places other than this, if you agree with omitting
"typename"
> in the comments

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4243: // other typename
Traits::X86OperandMem, so there should be no mention of
On 2015/07/07 00:00:18, jvoung wrote:
> similar, in a few places other than this, if you agree with omitting
"typename"
> in the comments

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:4482: // segment
On 2015/07/07 00:00:18, jvoung wrote:
> could fit rest of comment on this line too

Done.

https://codereview.chromium.org/1216933015/diff/50001/src/IceTargetLoweringX8...
src/IceTargetLoweringX86BaseImpl.h:5141: // level
On 2015/07/07 00:00:18, jvoung wrote:
> could fit rest of comment here

Done.

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

5 years, 5 months ago (2015-07-07 18:23:18 UTC) #8

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#new...
src/IceInstX8632.h:12: /// modified
On 2015/07/07 17:22:59, jvoung wrote:
> I don't know why, but the line-wrapping of comments seem to leave a single
word
> on a line.

It is because clang-format is stupid: it splits comment lines, but it does not
merge them.... :-/

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86Base.h
File src/IceInstX86Base.h (right):

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:1: //===- subzero/src/IceInstX86.h - Generic x86
instructions -*- C++ -*--===//
On 2015/07/07 17:23:00, jvoung wrote:
> The file is still called IceInstX86Base.h (and the header guard is X86BASE)?
> 
> (also, the //=== ===// header no longer fills the whole line)

Done.

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:13: /// X86
On 2015/07/07 17:23:00, jvoung wrote:
> comment block also has some single word lines

Done.

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:460: class InstX86BaseInplaceopGPR : public
InstX86Base<Machine> {
On 2015/07/07 17:23:00, jvoung wrote:
> Should these still be "Base"?

Yes, they should: these are "Base" classes to the actual X86 instruction. So, as
a "rule", non-Base classes are final.

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86Base.h#n...
src/IceInstX86Base.h:1208: : public InstX86BaseBinopGPR<Machine,
InstX86Base<Machine>::Add> {
On 2015/07/07 17:23:00, jvoung wrote:
> These Binops like Add, Adc, etc. used to be a typedef which is a bit shorter.
> E.g.,
> 
> template <class Machine> using InstX86Add = InstX86BaseBinopGPR<Machine,
> InstX86Base<Machine>::Add>;
> 
> What is the motivation for switching to this style? It looks like the create()
> methods have been moved to here too, instead of being part of
> InstX86BaseBinopGPR, returning a "InstX86Add *" instead of a
> "InstX86BaseBinopGPR *".

The motivation is that the custom emit methods that was specialized for each
typedef do not work with partial template specialization. Classes with **just**
create and ctors can become type aliases again, but then we would have a mix of
alias an actual classes.

Worse yet, given that the base classes are not abstract, having a create()
method in then could prove disastrous in cases when a custom emit is needed:

class Foo : public InstX86BaseSomething<...> {
public:
  void emit(Cfg *Func) const override {}

private:
  Foo(...) : InstX86BaseSomething<...>(...) {}
};

this would compile but it is wrong. The

InstX86BaseSomething::create

constructor would be invoked, and the code would compile, but the Foo::emit
method would not be invoked. This is why I moved to having all these classes
here instead of the typedefs.

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86BaseImpl.h
File src/IceInstX86BaseImpl.h (right):

https://codereview.chromium.org/1216933015/diff/110001/src/IceInstX86BaseImpl...
src/IceInstX86BaseImpl.h:1: //===- subzero/src/IceInstX86Impl.h - Generic X86
instructions -*- C++ -*=//
On 2015/07/07 17:23:00, jvoung wrote:
> File is still called BaseImpl.h?

Done.

https://codereview.chromium.org/1216933015/diff/110001/src/IceTargetLoweringX...
File src/IceTargetLoweringX8632Traits.h (right):

https://codereview.chromium.org/1216933015/diff/110001/src/IceTargetLoweringX...
src/IceTargetLoweringX8632Traits.h:605: // TODO(jpp): remove references to
SplitVariable from IceInstX86Base as 64bit
On 2015/07/07 17:23:00, jvoung wrote:
> SplitVariable -> VariableSplit

Done.

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).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages