Subzero: Use 'override' as appropriate for C++11.

Subzero: Use 'override' as appropriate for C++11.

Not necessary for the LLVM 3.5 merge, but nice to have anyway.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3930
R=jfb@chromium.org, jvoung@chromium.org

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

[Jim Stichnoth, 2014-09-26 05:01:08]

stichnot@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org

[Jim Stichnoth, 2014-09-26 05:07:08]

https://codereview.chromium.org/605123002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/605123002/diff/1/src/IceInst.h#newcode756
src/IceInst.h:756: void emit(const Cfg *Func) const override = 0;
"... override = 0" looks weird to me.  What's the appropriate thing here?  There
is an Inst::emit() method with an implementation.  I want to make sure
InstTarget is pure virtual.

[JF, 2014-09-26 05:42:03]

lgtm with one nit.

https://codereview.chromium.org/605123002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/605123002/diff/1/src/IceInst.h#newcode756
src/IceInst.h:756: void emit(const Cfg *Func) const override = 0;
On 2014/09/26 05:07:08, stichnot wrote:
> "... override = 0" looks weird to me.  What's the appropriate thing here? 
There
> is an Inst::emit() method with an implementation.  I want to make sure
> InstTarget is pure virtual.

Inst::emit has an implementation that's just llvm_unreachable, and InstTarget
doesn't have an implementation. Why not drop InstTarget::emit entirely, and make
Inst::emit pure virtual instead?

[Jim Stichnoth, 2014-09-26 13:04:05]

PTAL, especially Jan have a look at my emitIAS() changes.

https://codereview.chromium.org/605123002/diff/1/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/605123002/diff/1/src/IceInst.h#newcode756
src/IceInst.h:756: void emit(const Cfg *Func) const override = 0;
On 2014/09/26 05:42:02, JF wrote:
> On 2014/09/26 05:07:08, stichnot wrote:
> > "... override = 0" looks weird to me.  What's the appropriate thing here? 
> There
> > is an Inst::emit() method with an implementation.  I want to make sure
> > InstTarget is pure virtual.
> 
> Inst::emit has an implementation that's just llvm_unreachable, and InstTarget
> doesn't have an implementation. Why not drop InstTarget::emit entirely, and
make
> Inst::emit pure virtual instead?

I want to make provide a default llvm_unreachable definition of emit for
high-level instructions (because high-level instructions should all be gone by
code emission time), but make emit() pure virtual for InstTarget so that someone
who adds a new target instruction is forced to implement emit().

I think the right thing here is to insert the InstHighLevel class and have all
the high-level instructions inherit from it.  Inst::emit() becomes pure virtual,
and InstHighLevel::emit() gets the llvm_unreachable version, and no more
"override = 0;" weirdness.

[jvoung (off chromium), 2014-09-26 16:13:49]

LGTM

https://codereview.chromium.org/605123002/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/605123002/diff/20001/src/IceInst.h#newcode759
src/IceInst.h:759: void emitIAS(const Cfg *Func) const override { emit(Func); }
At some point we will have to rely on dump to notice these, and then this
becomes a nop.

[Jim Stichnoth, 2014-09-26 16:27:24]

https://codereview.chromium.org/605123002/diff/20001/src/IceInst.h
File src/IceInst.h (right):

https://codereview.chromium.org/605123002/diff/20001/src/IceInst.h#newcode759
src/IceInst.h:759: void emitIAS(const Cfg *Func) const override { emit(Func); }
On 2014/09/26 16:13:49, jvoung wrote:
> At some point we will have to rely on dump to notice these, and then this
> becomes a nop.

Now that you mention it, there's not much reason to emit Fake{Def,Use,Kill} at
all, since it's all available in the dump().  I'll remove it (or someone else
can...) at some point.

[Jim Stichnoth, 2014-09-26 16:28:50]

Committed patchset #2 (id:20001) manually as b56c8f4 (presubmit successful).