Fixes the X86 Base template.

Fixes the X86 Base template.

It turns out that using using TargetLowering::<member> causes problems when compiling with g++. The problem was fixed by using
Machine:: instead, where Machine is the template parameter. With name-dependent identifier, g++ does the right thing.

BUG= None
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=36087cd4e098def6c9e7dfc9072edcd34888fff6

[John, 2015-06-24 20:27:04]

jpp@chromium.org changed reviewers:
+ stichnot@chromium.org

[Jim Stichnoth, 2015-06-24 21:58:47]

Otherwise LGTM.

(btw, I think maybe you forgot to send email for this CL)

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX8632....
src/IceTargetLoweringX8632.cpp:88: static const uint32_t X86_MAX_XMM_ARGS = 4;
I'm curious why all these constexprs got downgraded to const?

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Base.h
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:34:
static_assert(std::is_base_of<::Ice::TargetLowering, Machine>::value, "Machine
template parameter must be a TargetLowering.");
80-col

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:592: Variable *Beacon = Func->template
makeVariable(IceType_i32);
This is completely non-obvious that the template keyword should be necessary. 
It's surely going to continue to cause problems in the future.  Maybe add a TODO
fix it?  I would probably change Cfg::makeVariable() to not be a template
function, then add a Cfg::addVariable() method that just does
Variables.push_back(Var), and then add a TargetX86 method that creates and adds
a SpillVariable.

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:2728: SpillVariable *SpillVar =
Func->template makeVariable<SpillVariable>(SrcType);
80-col

[John, 2015-06-24 22:56:25]

I was 100% positive I had mailed the CL... Sorry about that (yet again)

Please take another look.

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX8632.cpp
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX8632....
src/IceTargetLoweringX8632.cpp:88: static const uint32_t X86_MAX_XMM_ARGS = 4;
On 2015/06/24 21:58:46, stichnot wrote:
> I'm curious why all these constexprs got downgraded to const?

It is dangerous to have them as constexpr as they are used (indirectly) in the
TargetLoweringX86Base template. Some compiler bug could (albeit unlikely) see
those uses and assume they are regular identifiers. If that happens, we would
see cryptic link-time errors (cryptic in the sense that the identifier is
defined but the linker would still complain about not being able to see it.)
I'll let the compiler/linker figure out whether this should be optimized -- or I
could change the code so that this Trait is defined before the
IceTargetLoweringX86Base.h file is included. For the time being, I'd rather just
be conservative and keep things const. Do you mind?

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Base.h
File src/IceTargetLoweringX86Base.h (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86Base.h:34:
static_assert(std::is_base_of<::Ice::TargetLowering, Machine>::value, "Machine
template parameter must be a TargetLowering.");
On 2015/06/24 21:58:46, stichnot wrote:
> 80-col

Done.

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:592: Variable *Beacon = Func->template
makeVariable(IceType_i32);
On 2015/06/24 21:58:46, stichnot wrote:
> This is completely non-obvious that the template keyword should be necessary. 
> It's surely going to continue to cause problems in the future.  Maybe add a
TODO
> fix it?  I would probably change Cfg::makeVariable() to not be a template
> function, then add a Cfg::addVariable() method that just does
> Variables.push_back(Var), and then add a TargetX86 method that creates and
adds
> a SpillVariable.

I totally agree that this use of template is non-obvious (I personally had never
seen it.) It turns out that **most** times LLVM will complain about it, with a
nice error message pointing out where the template keyword should be used.

"Fixing" this is not really possible, as this is a language feature rather than
a bug, so any code invoking template functions without <Args...> (or perhaps <>)
need the keyword.

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:2728: SpillVariable *SpillVar =
Func->template makeVariable<SpillVariable>(SrcType);
On 2015/06/24 21:58:46, stichnot wrote:
> 80-col

Done.

[Jim Stichnoth, 2015-06-24 23:02:08]

still lgtm

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:592: Variable *Beacon = Func->template
makeVariable(IceType_i32);
On 2015/06/24 22:56:25, John wrote:
> On 2015/06/24 21:58:46, stichnot wrote:
> > This is completely non-obvious that the template keyword should be
necessary. 
> > It's surely going to continue to cause problems in the future.  Maybe add a
> TODO
> > fix it?  I would probably change Cfg::makeVariable() to not be a template
> > function, then add a Cfg::addVariable() method that just does
> > Variables.push_back(Var), and then add a TargetX86 method that creates and
> adds
> > a SpillVariable.
> 
> I totally agree that this use of template is non-obvious (I personally had
never
> seen it.) It turns out that **most** times LLVM will complain about it, with a
> nice error message pointing out where the template keyword should be used.
> 
> "Fixing" this is not really possible, as this is a language feature rather
than
> a bug, so any code invoking template functions without <Args...> (or perhaps
<>)
> need the keyword.

I meant "fixing" our code such that makeVariable() is no longer a template
function.  And I'm specifically referring to the scenario where permissive LLVM
allows an invocation without the template keyword, and no one knows there's a
problem until g++ on the Windows bot complains.

[John, 2015-06-24 23:09:16]

Landing this in 3, 2, ...

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
File src/IceTargetLoweringX86BaseImpl.h (right):

https://codereview.chromium.org/1208663002/diff/1/src/IceTargetLoweringX86Bas...
src/IceTargetLoweringX86BaseImpl.h:592: Variable *Beacon = Func->template
makeVariable(IceType_i32);
On 2015/06/24 23:02:08, stichnot wrote:
> On 2015/06/24 22:56:25, John wrote:
> > On 2015/06/24 21:58:46, stichnot wrote:
> > > This is completely non-obvious that the template keyword should be
> necessary. 
> > > It's surely going to continue to cause problems in the future.  Maybe add
a
> > TODO
> > > fix it?  I would probably change Cfg::makeVariable() to not be a template
> > > function, then add a Cfg::addVariable() method that just does
> > > Variables.push_back(Var), and then add a TargetX86 method that creates and
> > adds
> > > a SpillVariable.
> > 
> > I totally agree that this use of template is non-obvious (I personally had
> never
> > seen it.) It turns out that **most** times LLVM will complain about it, with
a
> > nice error message pointing out where the template keyword should be used.
> > 
> > "Fixing" this is not really possible, as this is a language feature rather
> than
> > a bug, so any code invoking template functions without <Args...> (or perhaps
> <>)
> > need the keyword.
> 
> I meant "fixing" our code such that makeVariable() is no longer a template
> function.  And I'm specifically referring to the scenario where permissive
LLVM
> allows an invocation without the template keyword, and no one knows there's a
> problem until g++ on the Windows bot complains.

Got it. I added a TODO(jpp) in IceCfg.h

[John, 2015-06-24 23:16:18]

Committed patchset #3 (id:40001) manually as
36087cd4e098def6c9e7dfc9072edcd34888fff6 (tree was closed).